Re: [PATCH] doc: fix merge-base ASCII art tab spacing

[Philip Oakley]

From: "Junio C Hamano" 

"Philip Oakley"  writes:


It appears that acciidoctor sees the art as being a separated
mono-spaced block, with border/background as locally
appropriate. While the asciidoc looks to simply change to mono-spaced
text.


FWIW, I changed my mind and your patch is now queued on 'next'.

Thanks.


Many thanks

Philip

Re: [PATCH] daemon: detect and reject too-long paths

[Jeff King]

On Sat, Oct 22, 2016 at 12:59:38AM -0400, Jeff King wrote:

> When we are checking the path via path_ok(), we use some
> fixed PATH_MAX buffers. We write into them via snprintf(),
> so there's no possibility of overflow, but it does mean we
> may silently truncate the path, leading to potentially
> confusing errors when the partial path does not exist.
> 
> We're better off to reject the path explicitly.
> 
> Signed-off-by: Jeff King 
> ---
> Another option would be to switch to strbufs here. That potentially
> introduces cases where a client can convince us to just keep allocating
> memory, but I don't think so in practice; the paths and interpolated
> data items all have to come in 64K pkt-lines, which places a hard
> limit. This is a much more minimal change, though, and I don't hear
> anybody complaining about the inability to use large paths.

For reference, the switch to dynamic memory looks something like this.
We don't even need strbufs, and we can get rid of the static variables
entirely (they weren't about buffer reuse, but just about extending the
lifetime past the return value).

Though we do have to add some free()s to avoid leaking error cases, this
looks simpler to me (the return value _is_ leaked in the success case,
because the caller doesn't know if we returned the original value or a
newly allocated one. In practice it doesn't matter because we call this
function once per process; compare to the 8K of BSS being wasted in the
original).

diff --git a/daemon.c b/daemon.c
index 425aad0507..4575ce5 100644
--- a/daemon.c
+++ b/daemon.c
@@ -158,8 +158,7 @@ static size_t expand_path(struct strbuf *sb, const char 
*placeholder, void *ctx)
 
 static const char *path_ok(const char *directory, struct hostinfo *hi)
 {
-   static char rpath[PATH_MAX];
-   static char interp_path[PATH_MAX];
+   char *to_free = NULL;
const char *path;
const char *dir;
 
@@ -187,9 +186,9 @@ static const char *path_ok(const char *directory, struct 
hostinfo *hi)
namlen = slash - dir;
restlen -= namlen;
loginfo("userpath <%s>, request <%s>, namlen %d, 
restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
-   snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
-namlen, dir, user_path, restlen, slash);
-   dir = rpath;
+   dir = to_free = xstrfmt("%.*s/%s%.*s",
+   namlen, dir, user_path,
+   restlen, slash);
}
}
else if (interpolated_path && hi->saw_extended_args) {
@@ -207,11 +206,8 @@ static const char *path_ok(const char *directory, struct 
hostinfo *hi)
 
strbuf_expand(_path, interpolated_path,
  expand_path, );
-   strlcpy(interp_path, expanded_path.buf, PATH_MAX);
-   strbuf_release(_path);
-   loginfo("Interpolated dir '%s'", interp_path);
-
-   dir = interp_path;
+   dir = to_free = strbuf_detach(_path, NULL);
+   loginfo("Interpolated dir '%s'", dir);
}
else if (base_path) {
if (*dir != '/') {
@@ -219,8 +215,7 @@ static const char *path_ok(const char *directory, struct 
hostinfo *hi)
logerror("'%s': Non-absolute path denied (base-path 
active)", dir);
return NULL;
}
-   snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
-   dir = rpath;
+   dir = to_free = xstrfmt("%s%s", base_path, dir);
}
 
path = enter_repo(dir, strict_paths);
@@ -229,12 +224,15 @@ static const char *path_ok(const char *directory, struct 
hostinfo *hi)
 * if we fail and base_path_relaxed is enabled, try without
 * prefixing the base path
 */
+   free(to_free);
+   to_free = NULL;
dir = directory;
path = enter_repo(dir, strict_paths);
}
 
if (!path) {
logerror("'%s' does not appear to be a git repository", dir);
+   free(to_free);
return NULL;
}
 
@@ -265,6 +263,7 @@ static const char *path_ok(const char *directory, struct 
hostinfo *hi)
}
 
logerror("'%s': not in whitelist", path);
+   free(to_free);
return NULL;/* Fallthrough. Deny by default */
 }
 

Re: [PATCH 4/3] test-lib: bail out when "-v" used under "prove"

[Jacob Keller]

On Fri, Oct 21, 2016 at 9:45 PM, Jeff King  wrote:
> I thought I'd just knock this out in 5 minutes before I forgot about it.
> But as with so many things, getting it right proved slightly harder than
> I thought.

Always seems to be that way, doesn't it?

> But I did learn about TAP's "Bail out!" directive. And
> apparently you can pass it back arbitrary YAML (!). And the "--verbose"
> output really is violating the spec, and they claim that Test::Harness
> will eventually be tightened to complain (though that was in 2007, and
> it still hasn't happened, so...).
>
> Anyway. Here is the patch I came up with (on top of the others).
>

Nice.

> -- >8 --
> Subject: test-lib: bail out when "-v" used under "prove"
>
> When there is a TAP harness consuming the output of our test
> scripts, the "--verbose" breaks the output by mingling
> test command output with TAP. Because the TAP::Harness
> module used by "prove" is fairly lenient, this _usually_
> works, but it violates the spec, and things get very
> confusing if the commands happen to output a line that looks
> like TAP (e.g., the word "ok" on its own line).
>
> Let's detect this situation and complain. Just calling
> error() isn't great, though; prove will tell us that the
> script failed, but the message doesn't make it through to
> the user. Instead, we can use the special TAP signal "Bail
> out!". This not only shows the message to the user, but
> instructs the harness to stop running the tests entirely.
> This is exactly what we want here, as the problem is in the
> command-line options, and every test script would produce
> the same error.
>
> The result looks like this (the first "Bailout called" line
> is in red if prove uses color on your terminal):
>
>  $ make GIT_TEST_OPTS='--verbose --tee'
>  rm -f -r 'test-results'
>  *** prove ***
>  Bailout called.  Further testing stopped:  verbose mode forbidden under TAP 
> harness; try --verbose-log
>  FAILED--Further testing stopped: verbose mode forbidden under TAP harness; 
> try --verbose-log
>  Makefile:39: recipe for target 'prove' failed
>  make: *** [prove] Error 255
>

Nice that makes sense.

> Signed-off-by: Jeff King 
> ---
>  t/test-lib.sh | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 85946ec40d..b859db61ac 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -321,6 +321,16 @@ say () {
> say_color info "$*"
>  }
>
> +if test -n "$HARNESS_ACTIVE"
> +then
> +   if test "$verbose" = t || test -n "$verbose_only"
> +   then
> +   printf 'Bail out! %s\n' \
> +'verbose mode forbidden under TAP harness; try --verbose-log'
> +   exit 1
> +   fi
> +fi
> +

Not too much code, so that's good. I like it.

Thanks,
Jake

>  test "${test_description}" != "" ||
>  error "Test script did not set test_description."
>
> --
> 2.10.1.776.ge0e381e
>

Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically

[Jeff King]

On Fri, Oct 21, 2016 at 09:39:45PM -0700, Junio C Hamano wrote:

> And this is the final one.
> 
> -- >8 --
> From: Junio C Hamano 
> Date: Fri, 21 Oct 2016 21:33:06 -0700
> Subject: [PATCH] transport: compute summary-width dynamically
> 
> Now all that is left to do is to actually iterate over the refs
> and measure the display width needed to show their abbreviation.

I think we crossed emails. :) This is obviously correct, if we don't
mind paying the find_unique_abbrev cost twice for each sha1.

>  int transport_summary_width(const struct ref *refs)
>  {
> - return (2 * FALLBACK_DEFAULT_ABBREV + 3);
> + int maxw;
> +
> + for (maxw = -1; refs; refs = refs->next) {
> + maxw = measure_abbrev(>old_oid, maxw);
> + maxw = measure_abbrev(>new_oid, maxw);
> + }

This is a minor style nit, but I think it's better to avoid mixing
unrelated bits between the initialization, condition, and iteration bits
of a for loop. IOW:

  int maxw = -1;

  for (; refs; refs = refs->next) {
...
  }

makes the for-loop _just_ about iteration, and it is more clear that the
manipulation of "maxw" is a side effect of the loop that is valid after
it finishes.

Though in this particular case, the loop and the whole function are
short enough that I don't care that much. Just raising a philosophical
point. :)

-Peff

[PATCH] daemon: detect and reject too-long paths

[Jeff King]

When we are checking the path via path_ok(), we use some
fixed PATH_MAX buffers. We write into them via snprintf(),
so there's no possibility of overflow, but it does mean we
may silently truncate the path, leading to potentially
confusing errors when the partial path does not exist.

We're better off to reject the path explicitly.

Signed-off-by: Jeff King 
---
Another option would be to switch to strbufs here. That potentially
introduces cases where a client can convince us to just keep allocating
memory, but I don't think so in practice; the paths and interpolated
data items all have to come in 64K pkt-lines, which places a hard
limit. This is a much more minimal change, though, and I don't hear
anybody complaining about the inability to use large paths.

 daemon.c | 25 +
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index 425aad0507..ff0fa583b0 100644
--- a/daemon.c
+++ b/daemon.c
@@ -160,6 +160,7 @@ static const char *path_ok(const char *directory, struct 
hostinfo *hi)
 {
static char rpath[PATH_MAX];
static char interp_path[PATH_MAX];
+   size_t rlen;
const char *path;
const char *dir;
 
@@ -187,8 +188,12 @@ static const char *path_ok(const char *directory, struct 
hostinfo *hi)
namlen = slash - dir;
restlen -= namlen;
loginfo("userpath <%s>, request <%s>, namlen %d, 
restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
-   snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
-namlen, dir, user_path, restlen, slash);
+   rlen = snprintf(rpath, sizeof(rpath), "%.*s/%s%.*s",
+   namlen, dir, user_path, restlen, slash);
+   if (rlen >= sizeof(rpath)) {
+   logerror("user-path too large: %s", rpath);
+   return NULL;
+   }
dir = rpath;
}
}
@@ -207,7 +212,15 @@ static const char *path_ok(const char *directory, struct 
hostinfo *hi)
 
strbuf_expand(_path, interpolated_path,
  expand_path, );
-   strlcpy(interp_path, expanded_path.buf, PATH_MAX);
+
+   rlen = strlcpy(interp_path, expanded_path.buf,
+  sizeof(interp_path));
+   if (rlen >= sizeof(interp_path)) {
+   logerror("interpolated path too large: %s",
+interp_path);
+   return NULL;
+   }
+
strbuf_release(_path);
loginfo("Interpolated dir '%s'", interp_path);
 
@@ -219,7 +232,11 @@ static const char *path_ok(const char *directory, struct 
hostinfo *hi)
logerror("'%s': Non-absolute path denied (base-path 
active)", dir);
return NULL;
}
-   snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
+   rlen = snprintf(rpath, sizeof(rpath), "%s%s", base_path, dir);
+   if (rlen >= sizeof(rpath)) {
+   logerror("base-path too large: %s", rpath);
+   return NULL;
+   }
dir = rpath;
}
 
-- 
2.10.1.776.ge0e381e

[PATCH 4/3] test-lib: bail out when "-v" used under "prove"

[Jeff King]

On Fri, Oct 21, 2016 at 09:15:28AM -0700, Jacob Keller wrote:

> >> > For $HARNESS_ACTIVE with _just_ "--verbose", I don't think it would be a
> >> > good idea to activate it. We should either silently ignore --verbose
> >> > then, or complain and die.
> >>
> >> We should probably do that to make sure people realize what might
> >> happen. I read your series and it has a good explanation of the
> >> possible alternatives. I like the approach you chose.
> >
> > Thanks. Do you want to make a patch on top of my series?
> 
> I am not sure I will find time to do it today, so it wouldn't be for a
> few more days.

I thought I'd just knock this out in 5 minutes before I forgot about it.
But as with so many things, getting it right proved slightly harder than
I thought. But I did learn about TAP's "Bail out!" directive. And
apparently you can pass it back arbitrary YAML (!). And the "--verbose"
output really is violating the spec, and they claim that Test::Harness
will eventually be tightened to complain (though that was in 2007, and
it still hasn't happened, so...).

Anyway. Here is the patch I came up with (on top of the others).

-- >8 --
Subject: test-lib: bail out when "-v" used under "prove"

When there is a TAP harness consuming the output of our test
scripts, the "--verbose" breaks the output by mingling
test command output with TAP. Because the TAP::Harness
module used by "prove" is fairly lenient, this _usually_
works, but it violates the spec, and things get very
confusing if the commands happen to output a line that looks
like TAP (e.g., the word "ok" on its own line).

Let's detect this situation and complain. Just calling
error() isn't great, though; prove will tell us that the
script failed, but the message doesn't make it through to
the user. Instead, we can use the special TAP signal "Bail
out!". This not only shows the message to the user, but
instructs the harness to stop running the tests entirely.
This is exactly what we want here, as the problem is in the
command-line options, and every test script would produce
the same error.

The result looks like this (the first "Bailout called" line
is in red if prove uses color on your terminal):

 $ make GIT_TEST_OPTS='--verbose --tee'
 rm -f -r 'test-results'
 *** prove ***
 Bailout called.  Further testing stopped:  verbose mode forbidden under TAP 
harness; try --verbose-log
 FAILED--Further testing stopped: verbose mode forbidden under TAP harness; try 
--verbose-log
 Makefile:39: recipe for target 'prove' failed
 make: *** [prove] Error 255

Signed-off-by: Jeff King 
---
 t/test-lib.sh | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 85946ec40d..b859db61ac 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -321,6 +321,16 @@ say () {
say_color info "$*"
 }
 
+if test -n "$HARNESS_ACTIVE"
+then
+   if test "$verbose" = t || test -n "$verbose_only"
+   then
+   printf 'Bail out! %s\n' \
+'verbose mode forbidden under TAP harness; try --verbose-log'
+   exit 1
+   fi
+fi
+
 test "${test_description}" != "" ||
 error "Test script did not set test_description."
 
-- 
2.10.1.776.ge0e381e

Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically

[Junio C Hamano]

Junio C Hamano  writes:

> Now we have identified three callchains that have a set of refs that
> they want to show their  object names in an aligned output,
> we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH
> with a helper function call to transport_summary_width() that takes
> the set of ref as a parameter.  This step does not yet iterate over
> the refs and compute, which is left as an exercise to the readers.

And this is the final one.

-- >8 --
From: Junio C Hamano 
Date: Fri, 21 Oct 2016 21:33:06 -0700
Subject: [PATCH] transport: compute summary-width dynamically

Now all that is left to do is to actually iterate over the refs
and measure the display width needed to show their abbreviation.

Signed-off-by: Junio C Hamano 
---
 transport.c | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/transport.c b/transport.c
index d4b8bf5f25..f1f95cf7c7 100644
--- a/transport.c
+++ b/transport.c
@@ -429,9 +429,25 @@ static int print_one_push_status(struct ref *ref, const 
char *dest, int count,
return 1;
 }
 
+static int measure_abbrev(const struct object_id *oid, int sofar)
+{
+   char hex[GIT_SHA1_HEXSZ + 1];
+   int w = find_unique_abbrev_r(hex, oid->hash, DEFAULT_ABBREV);
+
+   return (w < sofar) ? sofar : w;
+}
+
 int transport_summary_width(const struct ref *refs)
 {
-   return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+   int maxw;
+
+   for (maxw = -1; refs; refs = refs->next) {
+   maxw = measure_abbrev(>old_oid, maxw);
+   maxw = measure_abbrev(>new_oid, maxw);
+   }
+   if (maxw < 0)
+   maxw = FALLBACK_DEFAULT_ABBREV;
+   return (2 * maxw + 3);
 }
 
 void transport_print_push_status(const char *dest, struct ref *refs,
-- 
2.10.1-723-g2384e83bc3

Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically

[Jeff King]

On Fri, Oct 21, 2016 at 03:39:27PM -0700, Junio C Hamano wrote:

> Now we have identified three callchains that have a set of refs that
> they want to show their  object names in an aligned output,
> we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH
> with a helper function call to transport_summary_width() that takes
> the set of ref as a parameter.  This step does not yet iterate over
> the refs and compute, which is left as an exercise to the readers.

The final step could be something like this:

diff --git a/transport.c b/transport.c
index 4dac713063..c1eaa4a 100644
--- a/transport.c
+++ b/transport.c
@@ -443,7 +443,21 @@ static int print_one_push_status(struct ref *ref, const 
char *dest, int count,
 
 int transport_summary_width(const struct ref *refs)
 {
-   return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+   int max_abbrev;
+
+   /*
+* Computing the complete set of abbreviated sha1s is expensive just to
+* find their lengths, but we can at least find our real dynamic
+* minimum by picking an arbitrary sha1.
+*/
+   if (refs)
+   max_abbrev = strlen(find_unique_abbrev(refs->old_oid.hash,
+  DEFAULT_ABBREV));
+   else
+   max_abbrev = FALLBACK_DEFAULT_ABBREV;
+
+   /* 2 abbreviated sha1s, plus "..." in between */
+   return (2 * max_abbrev + 3);
 }
 
 void transport_print_push_status(const char *dest, struct ref *refs,


which produces reasonable results for me. But if we really wanted the
true value, I think we'd want to compute and store the abbreviated
sha1s, and then the refactoring done by your series probably isn't the
right direction.

I think we'd instead want to replace "struct strbuf *display" passed
down to update_local_ref() with something more like:

  struct ref_status_table {
struct ref_status_item {
char old_hash[GIT_SHA1_HEX + 1];
char new_hash[GIT_SHA1_HEX + 1];
char *remote_ref;
char *local_ref;
char *summary;
char code;
} *items;
size_t alloc, nr;
  };

and then format_display() would just add to the list (including calling
find_unique_abbrev()), and then at the end we'd call a function to show
them all.

That would also get rid of prepare_format_display(), as we could easily
walk over the prepared list before printing any of them (as opposed to
what that function does now, which is to walk over the ref map; that
requires that it know which refs are going to be printed).

-Peff

Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths

[Jeff King]

On Fri, Oct 21, 2016 at 11:23:53AM -0700, Junio C Hamano wrote:

> A request to "git:///", depending on , results
> in "directory" given to path_ok() in a bit different forms.  Namely,
> connect.c::parse_connect_url() gives
> 
> URL   directory
> git://site/nouser.git /nouser.git
> git://site/~nouser.git~nouser.git
> 
> by special casing "~user" syntax (in other words, a pathname that
> begins with a tilde _is_ special cased, and tilde is not considered
> a normal character that can be in a pathname).  Note the lack of
> leading slash in the second one.
> 
> Because that is how the shipped clients work, the daemon needs a bit
> of adjustment, because interpolation and base-path prefix codepaths
> wants to accept only paths that begin with a slash, and relies on
> the slash when interpolating it in the template or concatenating it
> to the base (e.g. roughly "sprintf(buf, "%s%s", base_path, dir)").
> 
> I came up with the following as a less invasive patch.  There no
> longer is the ase where "user-path not allowed" error is given,
> as treating tilde as just a normal pathname character even when it
> appears at the beginning is the whole point of this change.

Thanks for explaining this. It is rather gross, but I think your
less-invasive patch is the best we could do given the client behavior.
And it's more what I would have expected based on the original problem
description.

> As I said already, I am not sure if this is a good change, though.

I also am on the fence. I'm not sure I understand a compelling reason to
have a non-interpolated "~" in the repo path name. Sure, it's a
limitation, but why does anybody care?

> @@ -170,11 +171,11 @@ static const char *path_ok(const char *directory, 
> struct hostinfo *hi)
>   return NULL;
>   }
>  
> + if (!user_path && *dir == '~') {
> + snprintf(tilde_path, PATH_MAX, "/%s", dir);
> + dir = tilde_path;
> + }

I know you are following existing convention in this function to use an
unchecked snprintf(), but it makes me wonder what kind of mischief a
client could get up to by silently truncating via snprintf. This
function is, after all, supposed to be checking the quality of the
incoming path.

xsnprintf() is probably too blunt a hammer, but:

  if (snprintf(rpath, PATH_MAX, ...) >= PATH_MAX) {
logerror("path too long");
return NULL;
  }

in various places may be appropriate.

-Peff

Re: [PATCH] doc: fix merge-base ASCII art tab spacing

[Jeff King]

On Fri, Oct 21, 2016 at 10:26:29PM +0100, Philip Oakley wrote:

> > updating the source they work on.  Otherwise, the broken "doc-tool
> > stack" will keep producing broken output next time a source that
> > respects "tab is to skip to the next multiple of 8" rule is fed to
> > it, no?
> 
> By avoiding tabs *within the art* we would also be tolerant of those who may
> not have a set their tab spacing to 8 when viewing the raw text.
> 
> It's particularly the criss-cross diagram that needs fixed one way or
> another (for the doc/doctor differences).

I think the new asciidoctor correctly handles tabs within the art. The
earlier diagrams begin each line with a tab (to mark the pre-formatted
block), and then only some of the lines have additional tabs, and expect
those tabs to expand to 8 characters to line up with the other bits
(which is what caused a problem with earlier asciidoctor).

What is funny about that criss-cross diagram is that it actually chooses
different markers on each line to start the art: sometimes tabs and
sometimes spaces. And that seems to confuse even recent versions of
asciidoctor.

It may be that asciidoctor is wrong here, but I have to admit we are
venturing well into "what happens to work with asciidoc" territory, and
the right solution is just fixing the diagram (i.e., your patch).

-Peff

Re: [PATCH v5 0/8] allow non-trailers and multiple-line trailers

[Stefan Beller]

On Fri, Oct 21, 2016 at 4:59 PM, Junio C Hamano  wrote:
> On Fri, Oct 21, 2016 at 10:54 AM, Jonathan Tan  
> wrote:
>> I've updated patch 5/8 to use strcspn and to pass in the list of
>> separators, meaning that we no longer accept '=' in file input (and also
>> updated its commit message accordingly).
>
> Thanks for a pleasant read. Queued.
>
> Hopefully this is ready for 'next' now.

I also just read through and was about to say the same.

[PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"

[Stefan Beller]

When adding a submodule via "git submodule add ",
the relative url applies to the superprojects remote. When the
superproject was cloned via "git clone . super", the remote url
is ending with '/.'.

The logic to construct the relative urls is not smart enough to
detect that the ending /. is referring to the directory itself
but rather treats it like any other relative path, i.e.

path/to/dir/. + ../relative/path/to/submodule

would result in

path/to/dir/relative/path/to/submodule

and not omit the "dir" as you may expect.

As in a later patch we'll normalize the remote url before the
computation of relative urls takes place, we need to first get our
test suite in a shape with normalized urls only, which is why we should
refrain from cloning from '.'

Signed-off-by: Stefan Beller 
---
 t/t7064-wtstatus-pv2.sh  | 9 ++---
 t/t7403-submodule-sync.sh| 3 ++-
 t/t7406-submodule-update.sh  | 6 --
 t/t7407-submodule-foreach.sh | 3 ++-
 t/t7506-status-submodule.sh  | 3 ++-
 5 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index 3012a4d..95514bb 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -330,7 +330,8 @@ test_expect_success 'verify UU (edit-edit) conflict' '
 test_expect_success 'verify upstream fields in branch header' '
git checkout master &&
test_when_finished "rm -rf sub_repo" &&
-   git clone . sub_repo &&
+   git clone "$(pwd)" sub_repo &&
+   git -C sub_repo config --unset remote.origin.url &&
(
## Confirm local master tracks remote master.
cd sub_repo &&
@@ -392,8 +393,10 @@ test_expect_success 'verify upstream fields in branch 
header' '
 
 test_expect_success 'create and add submodule, submodule appears clean (A. 
S...)' '
git checkout master &&
-   git clone . sub_repo &&
-   git clone . super_repo &&
+   git clone "$(pwd)" sub_repo &&
+   git -C sub_repo config --unset remote.origin.url &&
+   git clone "$(pwd)" super_repo &&
+   git -C super_repo config --unset remote.origin.url &&
(   cd super_repo &&
git submodule add ../sub_repo sub1 &&
 
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 0726799..6d85391 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -15,7 +15,8 @@ test_expect_success setup '
git add file &&
test_tick &&
git commit -m upstream &&
-   git clone . super &&
+   git clone "$(pwd)" super &&
+   git -C super config --unset remote.origin.url &&
git clone super submodule &&
(
cd submodule &&
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 64f322c..9430c2a 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -26,7 +26,8 @@ test_expect_success 'setup a submodule tree' '
git add file &&
test_tick &&
git commit -m upstream &&
-   git clone . super &&
+   git clone "$(pwd)" super &&
+   git -C super config --unset remote.origin.url &&
git clone super submodule &&
git clone super rebasing &&
git clone super merging &&
@@ -64,7 +65,8 @@ test_expect_success 'setup a submodule tree' '
 test_tick &&
 git commit -m "none"
) &&
-   git clone . recursivesuper &&
+   git clone "$(pwd)" recursivesuper &&
+   git -C recursivesuper config --unset remote.origin.url &&
( cd recursivesuper
 git submodule add ../super super
)
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf..257e817 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -17,7 +17,8 @@ test_expect_success 'setup a submodule tree' '
git add file &&
test_tick &&
git commit -m upstream &&
-   git clone . super &&
+   git clone "$(pwd)" super &&
+   git -C super config --unset remote.origin.url &&
git clone super submodule &&
(
cd super &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 74cb6b8..62a99bc 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -197,7 +197,8 @@ A  sub1
 EOF
 
 test_expect_success 'status with merge conflict in .gitmodules' '
-   git clone . super &&
+   git clone "$(pwd)" super &&
+   git -C super config --unset remote.origin.url &&
test_create_repo_with_commit sub1 &&
test_tick &&
test_create_repo_with_commit sub2 &&
-- 
2.10.1.507.g2a9098a

Re: [PATCH v5 0/8] allow non-trailers and multiple-line trailers

[Junio C Hamano]

On Fri, Oct 21, 2016 at 10:54 AM, Jonathan Tan  wrote:
> I've updated patch 5/8 to use strcspn and to pass in the list of
> separators, meaning that we no longer accept '=' in file input (and also
> updated its commit message accordingly).

Thanks for a pleasant read. Queued.

Hopefully this is ready for 'next' now.

[PATCH 3/3] submodule--helper: normalize funny urls

[Stefan Beller]

The remote URL for the submodule can be specified relative
to the URL of the superproject in .gitmodules.  A top-level
git://site.xz/toplevel.git can specify in its .gitmodules

[submodule "sub"]
url = ../submodule.git
path = sub

to say that git://site.xz/submodule.git is where the
submodule bound at its "sub/" is found.

However, when the toplevel is cloned like this:

git clone git://site.xz/toplevel.git/. top

i.e. the toplevel specifies its URL with trailing "/.", the
code set the URL to git://site.xz/toplevel.git/submodule.git
for the submodule, which is nonsense.  This was because the
logic failed to treat trailing "/." any differently from
trailing "/" when resolving a
relative URL "../" off of it.  Stripping "/." at
the end does *not* take you one level up, even though
stripping "/" does!

Some users may rely on this by always cloning with '/.' and having
an additional '../' in the relative path for the submodule, and this
patch breaks them. So why introduce this patch?

The fix in c12922024 (submodule: ignore trailing slash on superproject
URL, 2016-10-10) and prior discussion revealed, that Git and Git
for Windows treat URLs differently, as currently Git for Windows
strips off a trailing dot from paths when calling a Git binary
unlike when running a shell. Which means Git for Windows is already
doing the right thing for the case mentioned above, but it would fail
our current tests, that test for the broken behavior and it would
confuse users working across platforms. So we'd rather fix it
in Git to ignore any of these trailing no ops in the path properly.

We never produce the URLs with a trailing '/.' in Git ourselves,
they come to us, because the user used it as the URL for cloning
a superproject. Normalize these paths.

The test 3600 needs a slight adaption as well, and was not covered by
the previous patch, because the setup of the submodule in this test
is different than the "clone . super" pattern that was fixed in the
previous patch.

The url configured for the submodule path submod is "./.", which
gets normalized with this patch, such that the nested submodule
'subsubmod' doesn't resolve its path properly via '../'.
In later tests we do not need the url any more as testing of
removal of the config including url happens in early tests, so the
easieast fix for that test is to just make the submodule its own
authoritative source of truth by removing the remote url.

Helped-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 48 +
 t/t0060-path-utils.sh   | 11 +++
 t/t3600-rm.sh   |  1 +
 3 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5..21e2e2a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -76,6 +76,29 @@ static int chop_last_dir(char **remoteurl, int is_relative)
return 0;
 }
 
+static void strip_url_ending(char *url, size_t *len_)
+{
+   size_t len = len_ ? *len_ : strlen(url);
+
+   for (;;) {
+   if (len > 1 && is_dir_sep(url[len - 2]) && url[len - 1] == '.') 
{
+   url[len-2] = '\0';
+   len -= 2;
+   continue;
+   }
+   if (len > 0 && is_dir_sep(url[len-1])) {
+   url[len-1] = '\0';
+   len--;
+   continue;
+   }
+
+   break;
+   }
+
+   if (len_)
+   *len_ = len;
+}
+
 /*
  * The `url` argument is the URL that navigates to the submodule origin
  * repo. When relative, this URL is relative to the superproject origin
@@ -93,14 +116,16 @@ static int chop_last_dir(char **remoteurl, int is_relative)
  * the superproject working tree otherwise.
  *
  * NEEDSWORK: This works incorrectly on the domain and protocol part.
- * remote_url  url  outcome  expectation
- * http://a.com/b  ../c http://a.com/c   as is
- * http://a.com/b/ ../c http://a.com/c   same as previous line, but
- *   ignore trailing slash in 
url
- * http://a.com/b  ../../c  http://c error out
- * http://a.com/b  ../../../c   http:/c  error out
- * http://a.com/b  ../../../../chttp:c   error out
- * http://a.com/b  ../../../../../c.:c   error out
+ * remote_url   url  outcome  expectation
+ * http://a.com/b   ../c http://a.com/c   as is
+ * http://a.com/b/  ../c http://a.com/c   same as previous line, 
but
+ *ignore trailing '/' in 
url
+ * http://a.com/b/. ../c http://a.com/c   same as previous line, 
but
+ *  

[PATCH 1/3] t7506: fix diff order arguments in test_cmp

[Stefan Beller]

Fix the argument order for test_cmp. When given the expected
result first the diff shows the actual output with '+' and the
expectation with '-', which is the convention for our tests.

Signed-off-by: Stefan Beller 
---
 t/t7506-status-submodule.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index d31b34d..74cb6b8 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -260,7 +260,7 @@ test_expect_success 'diff with merge conflict in 
.gitmodules' '
cd super &&
git diff >../diff_actual 2>&1
) &&
-   test_cmp diff_actual diff_expect
+   test_cmp diff_expect diff_actual
 '
 
 test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
@@ -268,7 +268,7 @@ test_expect_success 'diff --submodule with merge conflict 
in .gitmodules' '
cd super &&
git diff --submodule >../diff_submodule_actual 2>&1
) &&
-   test_cmp diff_submodule_actual diff_submodule_expect
+   test_cmp diff_submodule_expect diff_submodule_actual
 '
 
 test_done
-- 
2.10.1.507.g2a9098a

[PATCH 0/3] Fix submodule url issues

[Stefan Beller]

previous patch: 
http://public-inbox.org/git/20161018210623.32696-1-sbel...@google.com/

First fix our test suite in patch 2,
then patch 3 is a resend of the previous patch to normalize the configured
remote.

Thanks,
Stefan

Stefan Beller (3):
  t7506: fix diff order arguments in test_cmp
  submodule tests: replace cloning from . by "$(pwd)"
  submodule--helper: normalize funny urls

 builtin/submodule--helper.c  | 48 +---
 t/t0060-path-utils.sh| 11 ++
 t/t3600-rm.sh|  1 +
 t/t7064-wtstatus-pv2.sh  |  9 ++---
 t/t7403-submodule-sync.sh|  3 ++-
 t/t7406-submodule-update.sh  |  6 --
 t/t7407-submodule-foreach.sh |  3 ++-
 t/t7506-status-submodule.sh  |  7 ---
 8 files changed, 62 insertions(+), 26 deletions(-)

-- 
2.10.1.507.g2a9098a

Re: generating combined diff without an existing merge commit

[Jacob Keller]

On Fri, Oct 21, 2016 at 3:41 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> Turns out that somehow I must have messed up my command because "git
>> diff   " does indeed do exactly what I
>> want.
>
> Thanks.  I was wondering why it didn't work for you, as the feature
> was done exactly to support that use case ;-)

I'm not really sure. It kept spitting out usage information to me, but
then I tried again with a fresh example and it worked as expected.

Thanks,
Jake

Re: [PATCH] doc: fix merge-base ASCII art tab spacing

[Junio C Hamano]

"Philip Oakley"  writes:

> It appears that acciidoctor sees the art as being a separated
> mono-spaced block, with border/background as locally
> appropriate. While the asciidoc looks to simply change to mono-spaced
> text.

FWIW, I changed my mind and your patch is now queued on 'next'.

Thanks.

Re: generating combined diff without an existing merge commit

[Junio C Hamano]

Jacob Keller  writes:

> Turns out that somehow I must have messed up my command because "git
> diff   " does indeed do exactly what I
> want.

Thanks.  I was wondering why it didn't work for you, as the feature
was done exactly to support that use case ;-)

[PATCH 2/3] fetch: pass summary_width down the callchain

[Junio C Hamano]

The leaf function on the "fetch" side that uses TRANSPORT_SUMMARY_WIDTH
constant is builtin/fetch.c::format_display() and it has two distinct
callchains.  The one that reports the primary result of fetch originates
at store_updated_refs(); the other one that reports the pruning of
the remote-tracking refs originates at prune_refs().

Teach these two places to pass summary_width down the callchain,
just like we did for the "push" side in the previous commit.

Signed-off-by: Junio C Hamano 
---
 builtin/fetch.c | 37 +
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index a9f12cc5cf..40696e5338 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -17,9 +17,6 @@
 #include "argv-array.h"
 #include "utf8.h"
 
-#define TRANSPORT_SUMMARY(x) \
-   (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
-
 static const char * const builtin_fetch_usage[] = {
N_("git fetch [] [ [...]]"),
N_("git fetch [] "),
@@ -569,9 +566,12 @@ static void print_compact(struct strbuf *display,
 
 static void format_display(struct strbuf *display, char code,
   const char *summary, const char *error,
-  const char *remote, const char *local)
+  const char *remote, const char *local,
+  int summary_width)
 {
-   strbuf_addf(display, "%c %-*s ", code, TRANSPORT_SUMMARY(summary));
+   int width = (summary_width + strlen(summary) - gettext_width(summary));
+
+   strbuf_addf(display, "%c %-*s ", code, width, summary);
if (!compact_format)
print_remote_to_local(display, remote, local);
else
@@ -583,7 +583,8 @@ static void format_display(struct strbuf *display, char 
code,
 static int update_local_ref(struct ref *ref,
const char *remote,
const struct ref *remote_ref,
-   struct strbuf *display)
+   struct strbuf *display,
+   int summary_width)
 {
struct commit *current = NULL, *updated;
enum object_type type;
@@ -597,7 +598,7 @@ static int update_local_ref(struct ref *ref,
if (!oidcmp(>old_oid, >new_oid)) {
if (verbosity > 0)
format_display(display, '=', _("[up to date]"), NULL,
-  remote, pretty_ref);
+  remote, pretty_ref, summary_width);
return 0;
}
 
@@ -611,7 +612,7 @@ static int update_local_ref(struct ref *ref,
 */
format_display(display, '!', _("[rejected]"),
   _("can't fetch in current branch"),
-  remote, pretty_ref);
+  remote, pretty_ref, summary_width);
return 1;
}
 
@@ -621,7 +622,7 @@ static int update_local_ref(struct ref *ref,
r = s_update_ref("updating tag", ref, 0);
format_display(display, r ? '!' : 't', _("[tag update]"),
   r ? _("unable to update local ref") : NULL,
-  remote, pretty_ref);
+  remote, pretty_ref, summary_width);
return r;
}
 
@@ -654,7 +655,7 @@ static int update_local_ref(struct ref *ref,
r = s_update_ref(msg, ref, 0);
format_display(display, r ? '!' : '*', what,
   r ? _("unable to update local ref") : NULL,
-  remote, pretty_ref);
+  remote, pretty_ref, summary_width);
return r;
}
 
@@ -670,7 +671,7 @@ static int update_local_ref(struct ref *ref,
r = s_update_ref("fast-forward", ref, 1);
format_display(display, r ? '!' : ' ', quickref.buf,
   r ? _("unable to update local ref") : NULL,
-  remote, pretty_ref);
+  remote, pretty_ref, summary_width);
strbuf_release();
return r;
} else if (force || ref->force) {
@@ -685,12 +686,12 @@ static int update_local_ref(struct ref *ref,
r = s_update_ref("forced-update", ref, 1);
format_display(display, r ? '!' : '+', quickref.buf,
   r ? _("unable to update local ref") : _("forced 
update"),
-  remote, pretty_ref);
+  remote, pretty_ref, summary_width);
strbuf_release();
return r;
} else {
format_display(display, '!', _("[rejected]"), 
_("non-fast-forward"),
-  remote, pretty_ref);
+  remote, pretty_ref, 

[PATCH 3/3] transport: allow summary-width to be computed dynamically

[Junio C Hamano]

Now we have identified three callchains that have a set of refs that
they want to show their  object names in an aligned output,
we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH
with a helper function call to transport_summary_width() that takes
the set of ref as a parameter.  This step does not yet iterate over
the refs and compute, which is left as an exercise to the readers.

Signed-off-by: Junio C Hamano 
---
 builtin/fetch.c | 4 ++--
 transport.c | 7 ++-
 transport.h | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 40696e5338..09813cd826 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -722,7 +722,7 @@ static int store_updated_refs(const char *raw_url, const 
char *remote_name,
char *url;
const char *filename = dry_run ? "/dev/null" : git_path_fetch_head();
int want_status;
-   int summary_width = TRANSPORT_SUMMARY_WIDTH;
+   int summary_width = transport_summary_width(ref_map);
 
fp = fopen(filename, "a");
if (!fp)
@@ -906,7 +906,7 @@ static int prune_refs(struct refspec *refs, int ref_count, 
struct ref *ref_map,
int url_len, i, result = 0;
struct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, 
ref_map);
char *url;
-   int summary_width = TRANSPORT_SUMMARY_WIDTH;
+   int summary_width = transport_summary_width(stale_refs);
const char *dangling_msg = dry_run
? _("   (%s will become dangling)")
: _("   (%s has become dangling)");
diff --git a/transport.c b/transport.c
index ec02b78924..d4b8bf5f25 100644
--- a/transport.c
+++ b/transport.c
@@ -429,6 +429,11 @@ static int print_one_push_status(struct ref *ref, const 
char *dest, int count,
return 1;
 }
 
+int transport_summary_width(const struct ref *refs)
+{
+   return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+}
+
 void transport_print_push_status(const char *dest, struct ref *refs,
  int verbose, int porcelain, unsigned int 
*reject_reasons)
 {
@@ -436,7 +441,7 @@ void transport_print_push_status(const char *dest, struct 
ref *refs,
int n = 0;
unsigned char head_sha1[20];
char *head;
-   int summary_width = TRANSPORT_SUMMARY_WIDTH;
+   int summary_width = transport_summary_width(refs);
 
head = resolve_refdup("HEAD", RESOLVE_REF_READING, head_sha1, NULL);
 
diff --git a/transport.h b/transport.h
index e783377e40..706d99e818 100644
--- a/transport.h
+++ b/transport.h
@@ -142,7 +142,7 @@ struct transport {
 #define TRANSPORT_PUSH_ATOMIC 8192
 #define TRANSPORT_PUSH_OPTIONS 16384
 
-#define TRANSPORT_SUMMARY_WIDTH (2 * FALLBACK_DEFAULT_ABBREV + 3)
+extern int transport_summary_width(const struct ref *refs);
 
 /* Returns a transport suitable for the url */
 struct transport *transport_get(struct remote *, const char *);
-- 
2.10.1-723-g2384e83bc3

[PATCH 0/3] fetch output is ugly in 'next'

[Junio C Hamano]

It turns out that there are three codepaths, all of which have a set
of refs that they want to show them using TRANSPORT_SUMMARY_WIDTH
constant.  The two preparatory steps turn the constant used at the
leaf level into a parameter that is passed down through these
callchains, and the last step introduces a helper function that can
be used to compute the appropriate width to be fed to these
callchains.

Junio C Hamano (3):
  transport: pass summary_width down the callchain
  fetch: pass summary_width down the callchain
  transport: allow summary-width to be computed dynamically

 builtin/fetch.c | 37 +--
 transport.c | 68 -
 transport.h |  2 +-
 3 files changed, 65 insertions(+), 42 deletions(-)

-- 
2.10.1-723-g2384e83bc3

[PATCH 1/3] transport: pass summary_width down the callchain

[Junio C Hamano]

The callchain that originates at transport_print_push_status()
eventually hits a single leaf function, print_ref_status(), that is
used to show from what old object to what new object a ref got
updated, and the width of the part that shows old and new object
names used a constant TRANSPORT_SUMMARY_WIDTH.

Teach the callchain to pass the width down from the top instead.
This allows a future enhancement to compute the necessary display
width before calling down this callchain.

Signed-off-by: Junio C Hamano 
---
 transport.c | 63 +
 1 file changed, 38 insertions(+), 25 deletions(-)

diff --git a/transport.c b/transport.c
index 94d6dc3725..ec02b78924 100644
--- a/transport.c
+++ b/transport.c
@@ -295,7 +295,9 @@ void transport_update_tracking_ref(struct remote *remote, 
struct ref *ref, int v
}
 }
 
-static void print_ref_status(char flag, const char *summary, struct ref *to, 
struct ref *from, const char *msg, int porcelain)
+static void print_ref_status(char flag, const char *summary,
+struct ref *to, struct ref *from, const char *msg,
+int porcelain, int summary_width)
 {
if (porcelain) {
if (from)
@@ -307,7 +309,7 @@ static void print_ref_status(char flag, const char 
*summary, struct ref *to, str
else
fprintf(stdout, "%s\n", summary);
} else {
-   fprintf(stderr, " %c %-*s ", flag, TRANSPORT_SUMMARY_WIDTH, 
summary);
+   fprintf(stderr, " %c %-*s ", flag, summary_width, summary);
if (from)
fprintf(stderr, "%s -> %s", 
prettify_refname(from->name), prettify_refname(to->name));
else
@@ -321,15 +323,16 @@ static void print_ref_status(char flag, const char 
*summary, struct ref *to, str
}
 }
 
-static void print_ok_ref_status(struct ref *ref, int porcelain)
+static void print_ok_ref_status(struct ref *ref, int porcelain, int 
summary_width)
 {
if (ref->deletion)
-   print_ref_status('-', "[deleted]", ref, NULL, NULL, porcelain);
+   print_ref_status('-', "[deleted]", ref, NULL, NULL,
+porcelain, summary_width);
else if (is_null_oid(>old_oid))
print_ref_status('*',
(starts_with(ref->name, "refs/tags/") ? "[new tag]" :
"[new branch]"),
-   ref, ref->peer_ref, NULL, porcelain);
+   ref, ref->peer_ref, NULL, porcelain, summary_width);
else {
struct strbuf quickref = STRBUF_INIT;
char type;
@@ -349,12 +352,14 @@ static void print_ok_ref_status(struct ref *ref, int 
porcelain)
strbuf_add_unique_abbrev(, ref->new_oid.hash,
 DEFAULT_ABBREV);
 
-   print_ref_status(type, quickref.buf, ref, ref->peer_ref, msg, 
porcelain);
+   print_ref_status(type, quickref.buf, ref, ref->peer_ref, msg,
+porcelain, summary_width);
strbuf_release();
}
 }
 
-static int print_one_push_status(struct ref *ref, const char *dest, int count, 
int porcelain)
+static int print_one_push_status(struct ref *ref, const char *dest, int count,
+int porcelain, int summary_width)
 {
if (!count) {
char *url = transport_anonymize_url(dest);
@@ -364,56 +369,60 @@ static int print_one_push_status(struct ref *ref, const 
char *dest, int count, i
 
switch(ref->status) {
case REF_STATUS_NONE:
-   print_ref_status('X', "[no match]", ref, NULL, NULL, porcelain);
+   print_ref_status('X', "[no match]", ref, NULL, NULL,
+porcelain, summary_width);
break;
case REF_STATUS_REJECT_NODELETE:
print_ref_status('!', "[rejected]", ref, NULL,
-"remote does not support 
deleting refs", porcelain);
+"remote does not support deleting refs",
+porcelain, summary_width);
break;
case REF_STATUS_UPTODATE:
print_ref_status('=', "[up to date]", ref,
-ref->peer_ref, NULL, 
porcelain);
+ref->peer_ref, NULL, porcelain, summary_width);
break;
case REF_STATUS_REJECT_NONFASTFORWARD:
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
-"non-fast-forward", porcelain);
+"non-fast-forward", porcelain, summary_width);
break;
case REF_STATUS_REJECT_ALREADY_EXISTS:
print_ref_status('!', 

Re: [BUG] fetch output is ugly in 'next'

[Junio C Hamano]

Jeff King  writes:

> The "right" fix is to queue up the list of ref updates to print, find
> the abbreviations for each, and then print them all in one shot, knowing
> ahead of time the size necessary to align them. This could also let us
> improve the name-alignment.

Heh, that was what I was starting to code while listening to
Jonathan and Stefan talk in a meeting ;-)

Re: generating combined diff without an existing merge commit

[Jacob Keller]

On Fri, Oct 21, 2016 at 2:40 PM, Jacob Keller  wrote:
> Hi,
>
> I recently determined that I can produce an interdiff for a series
> that handles rebasing nicely and shows the conflicts resolved when
> rebasing plus any other changes.
>
> The basic idea is something like the following, assuming that v1 is a
> tag that points to the first version, v2 is a tag that points to the
> rebased new version, and base is a tag that points to the new base of
> the series (ie: the upstream if the v2 is on a branch and has been
> fully rebased)
>
> git checkout v1
> git merge base
> #perform any further edits to get everything looking like v2
> git commit
> git show -cc HEAD
>
> This is also equivalent to the following without having to actually do
> the merge manually:
>
> git commit-tree v2^{head} -p v1 -p master -m "some merge message"
> git show 
> this nicely shows us the combined diff format which correctly shows
> any conflicts required to fix up during the rebase (which we already
> did because we have v2) and it also shows any *other* changes caused
> by v2 but without showing changes which we didn't actually make. (I
> think?)
>
> The result is that we can nicely see what was required to produce v2
> from v1 but without being cluttered by what changed in base.
>
> However, I have to actually generate the commit to do this. I am
> wondering if it is possible today to actually just do something like:
>
> git diffand get the result that I want?
>
> I've already started digging to see if I can do that but haven't found
> anything yet.
>
> Thanks,
> Jake

Turns out that somehow I must have messed up my command because "git
diff   " does indeed do exactly what I
want.

Thanks,
Jake

Re: [PATCH 2/3] test-lib: add --verbose-log option

[Jeff King]

On Fri, Oct 21, 2016 at 10:12:16AM -0700, Junio C Hamano wrote:

> > -if test "$verbose" = "t"
> > +if test "$verbose_log" = "t"
> > +then
> > +   exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
> > +elif test "$verbose" = "t"
> >  then
> > exec 4>&2 3>&1
> >  else
> 
> OK, unlike "verbose" case where we give 3 (saved stdout) to 1 and 4
> (saved stderr) to 2, we send 3 to the output file and 4 to the same.

Your comment made me double-check whether we ought to be separating the
two in any way. Because my statement earlier in the thread that the
--verbose output goes to stdout is not _entirely_ true. It goes to
stdout and stderr, as written by the various programs in the tests.

So:

  ./t-whatever.sh -v | less

isn't quite sufficient. You really do want "2>&1" in there to catch
everything.

But for the case of "--tee", we already do so, in order to make sure it
all goes to the logfile. And since this option always implies "--tee",
it is correct here to send "3" and "4" to the same place.

So the patch is correct. But when I said earlier that people might be
annoyed if we just sent --verbose output to stderr, that probably isn't
true. We could perhaps make:

  prove t-whatever.sh :: -v

"just work" by sending all of the verbose output to stderr. I don't
think it really matters, though. Nobody is doing that, because it's
pointless without "--tee".

-Peff

Re: [BUG] fetch output is ugly in 'next'

[Jeff King]

On Fri, Oct 21, 2016 at 09:50:49AM -0700, Junio C Hamano wrote:

> Duy Nguyen  writes:
> 
> > On Fri, Oct 21, 2016 at 7:11 PM, Duy Nguyen  wrote:
> >> Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something
> >> sensible should do it.
> >
> > Correction (if somebody will pick this up), it's
> > TRANSPORT_SUMMARY_WIDTH that needs to be adjusted, not those four.
> 
> Yes, it used to be and it still is (2 * DEFAULT_ABBREV + 3) but in
> the new world order where default-abbrev is often -1 the expression
> does not make much sense.
> 
> Perhaps something along this line?
> 
>  transport.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/transport.h b/transport.h
> index 18d2cf8275..5339fabbad 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -127,7 +127,7 @@ struct transport {
>  #define TRANSPORT_PUSH_CERT 2048
>  #define TRANSPORT_PUSH_ATOMIC 4096
>  
> -#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
> +#define TRANSPORT_SUMMARY_WIDTH (2 * (DEFAULT_ABBREV < 0 ? 7 : 
> DEFAULT_ABBREV) + 3)
>  #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - 
> gettext_width(x)), (x)

That doesn't apply on 'next', because we have already done the
equivalent there. :)

The right way to spell "7" is FALLBACK_DEFAULT_ABBREV, which is handled
by your 65acfeacaa.

The remaining issue is that the static abbreviation is not nearly long
enough for git.git anymore; the auto-abbrev feature bumps my repo to a
minimum of 10 characters (it may only be 9 on a fresh clone; I have a
couple remotes and active work in progress). So this isn't exactly a
regression; it has always been the case that we may mis-align when the
abbreviations ended up longer than the minimum. It's just that it didn't
happen all that often in most repos (but it probably did constantly in
linux.git).

The simplest band-aid fix would be to compute TRANSPORT_SUMMARY_WIDTH on
the fly, taking into account the minimum found by actually counting the
objects. That at least gets us back to where we were, with it mostly
working and occasionally ugly when there's an oddball collision (for
git.git anyway; it probably makes the kernel output much nicer).

The "right" fix is to queue up the list of ref updates to print, find
the abbreviations for each, and then print them all in one shot, knowing
ahead of time the size necessary to align them. This could also let us
improve the name-alignment.

-Peff

generating combined diff without an existing merge commit

[Jacob Keller]

Hi,

I recently determined that I can produce an interdiff for a series
that handles rebasing nicely and shows the conflicts resolved when
rebasing plus any other changes.

The basic idea is something like the following, assuming that v1 is a
tag that points to the first version, v2 is a tag that points to the
rebased new version, and base is a tag that points to the new base of
the series (ie: the upstream if the v2 is on a branch and has been
fully rebased)

git checkout v1
git merge base
#perform any further edits to get everything looking like v2
git commit
git show -cc HEAD

This is also equivalent to the following without having to actually do
the merge manually:

git commit-tree v2^{head} -p v1 -p master -m "some merge message"
git showand get the result that I want?

I've already started digging to see if I can do that but haven't found
anything yet.

Thanks,
Jake

Re: [PATCH] doc: fix merge-base ASCII art tab spacing

[Philip Oakley]

From: "Junio C Hamano" 

Philip Oakley  writes:


The doc-tool stack does not always respect the 'tab = 8 spaces' rule,
particularly the git-scm doc pages 
https://git-scm.com/docs/git-merge-base

and the Git generated html pages.


Sorry, but I do not understand this change.

https://git.github.io/htmldocs/git-merge-base.html is "Git generated
HTML page" and I do not see any breakage around the drawings this
patch touches, and the fp-path series does not touch these drawings,
either.



I'd been looking the Git for Windows output, which has the same breakage, 
rather than yours. Apologies for failing to check there.



If a broken "doc-tool stack" breaks the formatting, I'd prefer to
see that "doc-tool stack" fixed, instead of working around by


The doc-tool stack is question is asciidoctor. It looks like it is an 
explicit decision that the the 8 space tab substitution is deprecated in 
these case (see http://asciidoctor.org/docs/user-manual/#migrate-deprecated 
#96.3)


It appears that acciidoctor sees the art as being a separated mono-spaced 
block, with border/background as locally appropriate. While the asciidoc 
looks to simply change to mono-spaced text.



updating the source they work on.  Otherwise, the broken "doc-tool
stack" will keep producing broken output next time a source that
respects "tab is to skip to the next multiple of 8" rule is fed to
it, no?


By avoiding tabs *within the art* we would also be tolerant of those who may 
not have a set their tab spacing to 8 when viewing the raw text.


It's particularly the criss-cross diagram that needs fixed one way or 
another (for the doc/doctor differences).


The update of the asciidoctor version for git-scm, as reported by peff isn't 
sufficient for this case.


also cc'ing dscho as this breakage shows in GfW (issue 923)

Philip

Re: [PATCHv3] submodule--helper: normalize funny urls

[Stefan Beller]

On Tue, Oct 18, 2016 at 7:05 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> I am not sure.  Certainly we would want to make sure that the normal
>>> case (i.e. no funny trailing junk) to work correctly, but we do want
>>> to protect the fix from future breakage as well, no?
>>
>> Exactly. So not intermediate "root" that we clone from, but adapting the
>> relative URLs. Maybe half the broken tests can switch to 'root' and the 
>> others
>> go with the current behavior of cloning . to super.
>>>
>>> Perhaps we can do a preliminary step to update tests to primarily
>>> check the cases that do not involve URL with trailing "/." by either
>>> doing that double clone, or more preferrably, clone from "$(pwd)"
>>> and adjust the incorrect submodule reference that have been relying
>>> on the buggy behaviour.  With that "root" trick, the test would pass
>>> with or without the fix under discussion, right?
>>
>> I assume so, (not tested).
>
> OK.  Thanks for sticking with it.

Ok, the root trick works fine without the fix, however we preferrably
want to fix it
without double cloning, then the fix becomes a bit harder to follow:

instead of

git clone . super

we do

git clone "$(pwd)" super &&
git -C super config --unset remote.origin.url &&

instead, such that the relative urls work the same way.
(The super becomes its own authoritative source of truth with no upstream.
In that case the url is relative to the superproject, e.g.

git -C super submodule add ../submodule

will then resolve the relative path from super/../submodule to be just
as before, where
we bugily used $(git config remote.origin.url =
...super/.)/../submodule, which due to the
but resolved to the submodule as well (as /. was counted as a directory).

Re: Drastic jump in the time required for the test suite

[René Scharfe]

Am 21.10.2016 um 12:59 schrieb Duy Nguyen:

On Thu, Oct 20, 2016 at 11:40 PM, René Scharfe  wrote:

I get this on WSL with prove -j8:

Files=750, Tests=13657, 906 wallclock secs ( 8.51 usr 17.17 sys + 282.62 cusr 
3731.85 csys = 4040.15 CPU)

And this for a run on Debian inside a Hyper-V VM on the same system:

Files=759, Tests=13895, 99 wallclock secs ( 4.81 usr  1.06 sys + 39.70 cusr 
25.82 csys = 71.39 CPU)

All tests pass on master.


Thank you for doing this. 10 times slower is probably not worth
following up (though absolute numbers still look amazing, you have
some beefy machine there).


Thanks, but it's not too impressive: Xeon E3-1231 v3, Crucial BX100 SSD, 
8GB RAM.  I wonder how much faster a brand-new CPU with more RAM and a 
PCIe SSD would be..


René

Re: Drastic jump in the time required for the test suite

[René Scharfe]

Am 21.10.2016 um 15:10 schrieb Matthieu Moy:
> René Scharfe  writes:
> 
>> I get this on WSL with prove -j8:
>>
>> Files=750, Tests=13657, 906 wallclock secs ( 8.51 usr 17.17 sys + 282.62 
>> cusr 3731.85 csys = 4040.15 CPU)
>>
>> And this for a run on Debian inside a Hyper-V VM on the same system:
>>
>> Files=759, Tests=13895, 99 wallclock secs ( 4.81 usr  1.06 sys + 39.70 cusr 
>> 25.82 csys = 71.39 CPU)
> 
> What about the same without WSL on windows?

Files=755, Tests=14894, 1774 wallclock secs ( 9.31 usr  9.58 sys + 821.87 cusr 
1961.23 csys = 2801.99 CPU)

René

Re: RFC Failover url for fetches?

[Junio C Hamano]

Stefan Beller  writes:

> So when pushing it is possible to have multiple urls
> (remote..url) configured.
>
> When fetching only the first configured url is considered.
> Would it make sense to allow multiple urls and
> try them one by one until one works?

I do not think the two are related.  Pushing to multiple is not "I
want to update at least one of them" in the first place.

As to fetching from two or more places as "fallback", I am
moderately negative to add it as a dumb feature that does nothing
more than "My fetch from A failed, so let's blindly try it from B".
I'd prefer to keep the "My fetch from A is failing" knowledge near
the surface of end user's consciousness as a mechanism to pressure A
to fix it--that way everybody who is fetching from A benefits.
After all, doing "git remote add B" once (you'd need to tell the URL
for B anyway to Git) and issuing "git fetch B" after seeing your
regular "git fetch" fails once in a blue moon is not all that
cumbersome, I would think.

Some people _may_ have objection based on A and B going out of sync,
especially B may fall behind even yourself and cause non-ff errors,
but I personally am not worried about that, because when somebody
configures B as a fallback for A, there is an expectation that B is
kept reasonably up to date.  It would be a problem if some refs are
expected to be constantly rewound at A (e.g. 'pu' in my tree) and
configured to always force-fetch, though.  A stale B would silently
set such a branch in your repository back without failing.

Re: [PATCH 1/2] t9000-addresses: update expected results after fix

[Ramsay Jones]

On 21/10/16 17:48, Junio C Hamano wrote:
> Matthieu Moy  writes:
> 
>> e3fdbcc8e1 (parse_mailboxes: accept extra text after <...> address,
>> 2016-10-13) improved our in-house address parser and made it closer to
>> Mail::Address. As a consequence, some tests comparing it to
>> Mail::Address now pass, but e3fdbcc8e1 forgot to update the test.
>>
>> Signed-off-by: Matthieu Moy 
>> ---
> 
> Thanks.

Yep, thanks for looking into this Matthieu.

I applied these cleanly (to both next and pu) and tested
on Linux and cygwin.

Thanks again.

ATB,
Ramsay Jones

Re: [PATCH v5 00/27] Prepare the sequencer for the upcoming rebase -i patches

[Junio C Hamano]

Johannes Schindelin  writes:

> Changes vs v4:

I still do not understand (note that I am not saying "I do not
accept"--acceptance or rejection happens after an understandable
explanation is given, and "do not understand" means no such
explanation has been given yet) your justification behind adding a
technical debt to reimplement the author-script parser and not
sharing it with "git am" in 13/27.

As I pointed out, I think 22/27 is out of place in this series.

But other than these two points, the changes since v4 look minimum,
and they all looked good to me.

Thanks.

Re: [PATCH v5 22/27] sequencer: teach write_message() to append an optional LF

[Junio C Hamano]

Johannes Schindelin  writes:

> This commit prepares for future callers that will have a pointer/length
> to some text to be written that lacks an LF, yet an LF is desired.
> Instead of requiring the caller to append an LF to the buffer (and
> potentially allocate memory to do so), the write_message() function
> learns to append an LF at the end of the file.

As no existing callers need this, it probably is better left out and
added to the series that actually needs the new feature as a
preparatory step.

Re: [PATCH v5 19/27] sequencer: stop releasing the strbuf in write_message()

[Junio C Hamano]

Johannes Schindelin  writes:

> Nothing in the name "write_message()" suggests that the function
> releases the strbuf passed to it. So let's release the strbuf in the
> caller instead.
>
> Signed-off-by: Johannes Schindelin 
> ---

I agree that it makes quite a lot of sense from the point of view of
"taste in the API design".

>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index d74fdce..745c86f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -243,7 +243,6 @@ static int write_message(struct strbuf *msgbuf, const 
> char *filename)
>   return error_errno(_("Could not lock '%s'"), filename);
>   if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
>   return error_errno(_("Could not write to %s"), filename);
> - strbuf_release(msgbuf);
>   if (commit_lock_file(_file) < 0)
>   return error(_("Error wrapping up %s."), filename);
>  
> @@ -759,6 +758,7 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   free_commit_list(common);
>   free_commit_list(remotes);
>   }
> + strbuf_release();
>  
>   /*
>* If the merge was clean or if it failed due to conflict, we write

Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths

[Junio C Hamano]

Junio C Hamano  writes:

> Duy Nguyen  writes:
>
>> The amount of changes is unbelievable for fixing such a rare case
>> though. I wonder if we can just detect this in daemon.c and pass
>> "./~foo/bar" instead of "~foo/bar" to enter_repo() in non-strict mode
>> to "disable" expand_user_path(). If it works, it's much simpler
>> changes (even though a bit hacky)
>
> Conceptually, it ought to be updating the code that says "Does it
> begin with a tilde?  Then try to do user-path expansion and fail if
> that is not enabled and if it succeeds use the resulting directory"
> to "Is user-path enabled and does it begin with a tilde?  Then try
> to do user-path expansion and if it succeeds use the resulting
> directory".  Compared to that mental model we have with this
> codepath, I agree that the change does look quite invasive and
> large.
>
> It is OK for a change to be large, as long as the end result is
> easier to read and understand than the original.  I am undecided if
> that is the case with this patch, though.

I am still not convinced that this is a bugfix (as opposed to "add a
new feature to please Luke while regressing it for others"), but the
"this looks too invasive" made me look at the codepath involved.

There is this code in daemon.c::path_ok() that takes "dir" and ends
up calling enter_repo().

if (*dir == '~') {
if (!user_path) {
logerror("'%s': User-path not allowed", dir);
return NULL;
}

At first glance, it ought to be a single-liner

-   if (*dir == '~') {
+   if (user_path && *dir == '~') {

to make 'git://site/~foo.git" go to "/var/scm/~foo.git" when base
path is set to /var/scm/.  

Unfortunately that is not sufficient.

A request to "git:///", depending on , results
in "directory" given to path_ok() in a bit different forms.  Namely,
connect.c::parse_connect_url() gives

URL directory
git://site/nouser.git   /nouser.git
git://site/~nouser.git  ~nouser.git

by special casing "~user" syntax (in other words, a pathname that
begins with a tilde _is_ special cased, and tilde is not considered
a normal character that can be in a pathname).  Note the lack of
leading slash in the second one.

Because that is how the shipped clients work, the daemon needs a bit
of adjustment, because interpolation and base-path prefix codepaths
wants to accept only paths that begin with a slash, and relies on
the slash when interpolating it in the template or concatenating it
to the base (e.g. roughly "sprintf(buf, "%s%s", base_path, dir)").

I came up with the following as a less invasive patch.  There no
longer is the ase where "user-path not allowed" error is given,
as treating tilde as just a normal pathname character even when it
appears at the beginning is the whole point of this change.

As I said already, I am not sure if this is a good change, though.
 daemon.c  |  9 +
 t/t5570-git-daemon.sh | 11 +++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index afce1b9b7f..e560a535af 100644
--- a/daemon.c
+++ b/daemon.c
@@ -160,6 +160,7 @@ static const char *path_ok(const char *directory, struct 
hostinfo *hi)
 {
static char rpath[PATH_MAX];
static char interp_path[PATH_MAX];
+   char tilde_path[PATH_MAX];
const char *path;
const char *dir;
 
@@ -170,11 +171,11 @@ static const char *path_ok(const char *directory, struct 
hostinfo *hi)
return NULL;
}
 
+   if (!user_path && *dir == '~') {
+   snprintf(tilde_path, PATH_MAX, "/%s", dir);
+   dir = tilde_path;
+   }
if (*dir == '~') {
-   if (!user_path) {
-   logerror("'%s': User-path not allowed", dir);
-   return NULL;
-   }
if (*user_path) {
/* Got either "~alice" or "~alice/foo";
 * rewrite them to "~alice/%s" or
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 225a022e8a..b6d2b9a001 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -65,6 +65,17 @@ test_expect_success 'remote detects correct HEAD' '
)
 '
 
+test_expect_success 'tilde is a normal character without --user-path' '
+   nouser="~nouser.git" &&
+   nouser_repo="$GIT_DAEMON_DOCUMENT_ROOT_PATH/$nouser" &&
+   mkdir "$nouser_repo" &&
+   git -C "$nouser_repo" --bare init &&
+   >"$nouser_repo/git-daemon-export-ok" &&
+   git push "$nouser_repo" master:master &&
+
+   git ls-remote "$GIT_DAEMON_URL/$nouser"
+'
+
 test_expect_success 'prepare pack objects' '
cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo.git 
"$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&

RFC Failover url for fetches?

[Stefan Beller]

So when pushing it is possible to have multiple urls
(remote..url) configured.

When fetching only the first configured url is considered.
Would it make sense to allow multiple urls and
try them one by one until one works?

Background:
http://www.businessinsider.com/amazon-spotify-twitter-github-and-etsy-down-in-apparent-dns-attack-2016-10

I wanted to fetch and had to fix a url of a repo
to point to a working mirror.

Thanks,
Stefan

[PATCH v5 3/8] trailer: streamline trailer item create and add

[Jonathan Tan]

Currently, creation and addition (to a list) of trailer items are spread
across multiple functions. Streamline this by only having 2 functions:
one to parse the user-supplied string, and one to add the parsed
information to a list.

Signed-off-by: Jonathan Tan 
---
 trailer.c | 130 +-
 1 file changed, 60 insertions(+), 70 deletions(-)

diff --git a/trailer.c b/trailer.c
index 4e85aae..ae3972a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -500,10 +500,31 @@ static int git_trailer_config(const char *conf_key, const 
char *value, void *cb)
return 0;
 }
 
-static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char 
*trailer)
+static const char *token_from_item(struct trailer_item *item, char *tok)
+{
+   if (item->conf.key)
+   return item->conf.key;
+   if (tok)
+   return tok;
+   return item->conf.name;
+}
+
+static int token_matches_item(const char *tok, struct trailer_item *item, int 
tok_len)
+{
+   if (!strncasecmp(tok, item->conf.name, tok_len))
+   return 1;
+   return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
+}
+
+static int parse_trailer(struct strbuf *tok, struct strbuf *val,
+const struct conf_info **conf, const char *trailer)
 {
size_t len;
struct strbuf seps = STRBUF_INIT;
+   struct trailer_item *item;
+   int tok_len;
+   struct list_head *pos;
+
strbuf_addstr(, separators);
strbuf_addch(, '=');
len = strcspn(trailer, seps.buf);
@@ -523,74 +544,31 @@ static int parse_trailer(struct strbuf *tok, struct 
strbuf *val, const char *tra
strbuf_addstr(tok, trailer);
strbuf_trim(tok);
}
-   return 0;
-}
-
-static const char *token_from_item(struct trailer_item *item, char *tok)
-{
-   if (item->conf.key)
-   return item->conf.key;
-   if (tok)
-   return tok;
-   return item->conf.name;
-}
-
-static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
-char *tok, char *val)
-{
-   struct trailer_item *new = xcalloc(sizeof(*new), 1);
-   new->value = val ? val : xstrdup("");
-
-   if (conf_item) {
-   duplicate_conf(>conf, _item->conf);
-   new->token = xstrdup(token_from_item(conf_item, tok));
-   free(tok);
-   } else {
-   duplicate_conf(>conf, _conf_info);
-   new->token = tok;
-   }
-
-   return new;
-}
-
-static int token_matches_item(const char *tok, struct trailer_item *item, int 
tok_len)
-{
-   if (!strncasecmp(tok, item->conf.name, tok_len))
-   return 1;
-   return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
-}
-
-static struct trailer_item *create_trailer_item(const char *string)
-{
-   struct strbuf tok = STRBUF_INIT;
-   struct strbuf val = STRBUF_INIT;
-   struct trailer_item *item;
-   int tok_len;
-   struct list_head *pos;
-
-   if (parse_trailer(, , string))
-   return NULL;
-
-   tok_len = token_len_without_separator(tok.buf, tok.len);
 
/* Lookup if the token matches something in the config */
+   tok_len = token_len_without_separator(tok->buf, tok->len);
+   *conf = _conf_info;
list_for_each(pos, _head) {
item = list_entry(pos, struct trailer_item, list);
-   if (token_matches_item(tok.buf, item, tok_len))
-   return new_trailer_item(item,
-   strbuf_detach(, NULL),
-   strbuf_detach(, NULL));
+   if (token_matches_item(tok->buf, item, tok_len)) {
+   char *tok_buf = strbuf_detach(tok, NULL);
+   *conf = >conf;
+   strbuf_addstr(tok, token_from_item(item, tok_buf));
+   free(tok_buf);
+   break;
+   }
}
 
-   return new_trailer_item(NULL,
-   strbuf_detach(, NULL),
-   strbuf_detach(, NULL));
+   return 0;
 }
 
-static void add_trailer_item(struct list_head *head, struct trailer_item *new)
+static void add_trailer_item(struct list_head *head, char *tok, char *val,
+const struct conf_info *conf)
 {
-   if (!new)
-   return;
+   struct trailer_item *new = xcalloc(sizeof(*new), 1);
+   new->token = tok;
+   new->value = val;
+   duplicate_conf(>conf, conf);
list_add_tail(>list, head);
 }
 
@@ -599,21 +577,28 @@ static void process_command_line_args(struct list_head 
*arg_head,
 {
struct string_list_item *tr;
struct trailer_item *item;
+   struct strbuf tok = STRBUF_INIT;
+   struct 

[PATCH v5 8/8] trailer: support values folded to multiple lines

[Jonathan Tan]

Currently, interpret-trailers requires that a trailer be only on 1 line.
For example:

a: first line
   second line

would be interpreted as one trailer line followed by one non-trailer line.

Make interpret-trailers support RFC 822-style folding, treating those
lines as one logical trailer.

Signed-off-by: Jonathan Tan 
---
 Documentation/git-interpret-trailers.txt |   7 +-
 t/t7513-interpret-trailers.sh| 169 +++
 trailer.c|  45 ++--
 3 files changed, 211 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index 4966b5b..e99bda6 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -57,11 +57,12 @@ minus signs start the patch part of the message.
 
 When reading trailers, there can be whitespaces after the
 token, the separator and the value. There can also be whitespaces
-inside the token and the value.
+inside the token and the value. The value may be split over multiple lines with
+each subsequent line starting with whitespace, like the "folding" in RFC 822.
 
 Note that 'trailers' do not follow and are not intended to follow many
-rules for RFC 822 headers. For example they do not follow the line
-folding rules, the encoding rules and probably many other rules.
+rules for RFC 822 headers. For example they do not follow
+the encoding rules and probably many other rules.
 
 OPTIONS
 ---
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 3d94b3a..4dd1d7c 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -256,6 +256,175 @@ test_expect_success 'line with leading whitespace is not 
trailer' '
test_cmp expected actual
 '
 
+test_expect_success 'multiline field treated as one trailer for 25% check' '
+   q_to_tab >patch <<-\EOF &&
+
+   Signed-off-by: a 
+   name: value on
+   Qmultiple lines
+   this is not a trailer
+   this is not a trailer
+   this is not a trailer
+   this is not a trailer
+   this is not a trailer
+   this is not a trailer
+   EOF
+   q_to_tab >expected <<-\EOF &&
+
+   Signed-off-by: a 
+   name: value on
+   Qmultiple lines
+   this is not a trailer
+   this is not a trailer
+   this is not a trailer
+   this is not a trailer
+   this is not a trailer
+   this is not a trailer
+   name: value
+   EOF
+   git interpret-trailers --trailer "name: value" patch >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for placement' '
+   q_to_tab >patch <<-\EOF &&
+
+   another: trailer
+   name: value on
+   Qmultiple lines
+   another: trailer
+   EOF
+   q_to_tab >expected <<-\EOF &&
+
+   another: trailer
+   name: value on
+   Qmultiple lines
+   name: value
+   another: trailer
+   EOF
+   test_config trailer.name.where after &&
+   git interpret-trailers --trailer "name: value" patch >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for replacement' '
+   q_to_tab >patch <<-\EOF &&
+
+   another: trailer
+   name: value on
+   Qmultiple lines
+   another: trailer
+   EOF
+   q_to_tab >expected <<-\EOF &&
+
+   another: trailer
+   another: trailer
+   name: value
+   EOF
+   test_config trailer.name.ifexists replace &&
+   git interpret-trailers --trailer "name: value" patch >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for difference check' '
+   q_to_tab >patch <<-\EOF &&
+
+   another: trailer
+   name: first line
+   Qsecond line
+   another: trailer
+   EOF
+   test_config trailer.name.ifexists addIfDifferent &&
+
+   q_to_tab >trailer <<-\EOF &&
+   name: first line
+   Qsecond line
+   EOF
+   q_to_tab >expected <<-\EOF &&
+
+   another: trailer
+   name: first line
+   Qsecond line
+   another: trailer
+   EOF
+   git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+   test_cmp expected actual &&
+
+   q_to_tab >trailer <<-\EOF &&
+   name: first line
+   Qsecond line
+   EOF
+   q_to_tab >expected <<-\EOF &&
+
+   another: trailer
+   name: first line
+   

[PATCH v5 2/8] trailer: use list.h for doubly-linked list

[Jonathan Tan]

Replace the existing handwritten implementation of a doubly-linked list
in trailer.c with the functions and macros from list.h. This
significantly simplifies the code.

Signed-off-by: Jonathan Tan 
Signed-off-by: Ramsay Jones 
---
 trailer.c | 258 ++
 1 file changed, 91 insertions(+), 167 deletions(-)

diff --git a/trailer.c b/trailer.c
index 1f191b2..4e85aae 100644
--- a/trailer.c
+++ b/trailer.c
@@ -4,6 +4,7 @@
 #include "commit.h"
 #include "tempfile.h"
 #include "trailer.h"
+#include "list.h"
 /*
  * Copyright (c) 2013, 2014 Christian Couder 
  */
@@ -25,19 +26,24 @@ struct conf_info {
 static struct conf_info default_conf_info;
 
 struct trailer_item {
-   struct trailer_item *previous;
-   struct trailer_item *next;
+   struct list_head list;
char *token;
char *value;
struct conf_info conf;
 };
 
-static struct trailer_item *first_conf_item;
+static LIST_HEAD(conf_head);
 
 static char *separators = ":";
 
 #define TRAILER_ARG_STRING "$ARG"
 
+/* Iterate over the elements of the list. */
+#define list_for_each_dir(pos, head, is_reverse) \
+   for (pos = is_reverse ? (head)->prev : (head)->next; \
+   pos != (head); \
+   pos = is_reverse ? pos->prev : pos->next)
+
 static int after_or_end(enum action_where where)
 {
return (where == WHERE_AFTER) || (where == WHERE_END);
@@ -120,101 +126,49 @@ static void print_tok_val(FILE *outfile, const char 
*tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
 }
 
-static void print_all(FILE *outfile, struct trailer_item *first, int 
trim_empty)
+static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
 {
+   struct list_head *pos;
struct trailer_item *item;
-   for (item = first; item; item = item->next) {
+   list_for_each(pos, head) {
+   item = list_entry(pos, struct trailer_item, list);
if (!trim_empty || strlen(item->value) > 0)
print_tok_val(outfile, item->token, item->value);
}
 }
 
-static void update_last(struct trailer_item **last)
-{
-   if (*last)
-   while ((*last)->next != NULL)
-   *last = (*last)->next;
-}
-
-static void update_first(struct trailer_item **first)
-{
-   if (*first)
-   while ((*first)->previous != NULL)
-   *first = (*first)->previous;
-}
-
 static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct trailer_item *arg_tok,
- struct trailer_item **first,
- struct trailer_item **last)
-{
-   if (after_or_end(arg_tok->conf.where)) {
-   arg_tok->next = on_tok->next;
-   on_tok->next = arg_tok;
-   arg_tok->previous = on_tok;
-   if (arg_tok->next)
-   arg_tok->next->previous = arg_tok;
-   update_last(last);
-   } else {
-   arg_tok->previous = on_tok->previous;
-   on_tok->previous = arg_tok;
-   arg_tok->next = on_tok;
-   if (arg_tok->previous)
-   arg_tok->previous->next = arg_tok;
-   update_first(first);
-   }
+ struct trailer_item *arg_tok)
+{
+   if (after_or_end(arg_tok->conf.where))
+   list_add(_tok->list, _tok->list);
+   else
+   list_add_tail(_tok->list, _tok->list);
 }
 
 static int check_if_different(struct trailer_item *in_tok,
  struct trailer_item *arg_tok,
- int check_all)
+ int check_all,
+ struct list_head *head)
 {
enum action_where where = arg_tok->conf.where;
+   struct list_head *next_head;
do {
-   if (!in_tok)
-   return 1;
if (same_trailer(in_tok, arg_tok))
return 0;
/*
 * if we want to add a trailer after another one,
 * we have to check those before this one
 */
-   in_tok = after_or_end(where) ? in_tok->previous : in_tok->next;
+   next_head = after_or_end(where) ? in_tok->list.prev
+   : in_tok->list.next;
+   if (next_head == head)
+   break;
+   in_tok = list_entry(next_head, struct trailer_item, list);
} while (check_all);
return 1;
 }
 
-static void remove_from_list(struct trailer_item *item,
-struct trailer_item **first,
-struct trailer_item **last)
-{
-   struct trailer_item *next = 

[PATCH v5 7/8] trailer: forbid leading whitespace in trailers

[Jonathan Tan]

Currently, interpret-trailers allows leading whitespace in trailer
lines. This leads to false positives, especially for quoted lines or
bullet lists.

Forbid leading whitespace in trailers.

Signed-off-by: Jonathan Tan 
---
 Documentation/git-interpret-trailers.txt |  2 +-
 t/t7513-interpret-trailers.sh| 15 +++
 trailer.c|  2 +-
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index cf4c5ea..4966b5b 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -55,7 +55,7 @@ The group must either be at the end of the message or be the 
last
 non-whitespace lines before a line that starts with '---'. Such three
 minus signs start the patch part of the message.
 
-When reading trailers, there can be whitespaces before and after the
+When reading trailers, there can be whitespaces after the
 token, the separator and the value. There can also be whitespaces
 inside the token and the value.
 
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 003e90f..3d94b3a 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -241,6 +241,21 @@ test_expect_success 'with non-trailer lines only' '
test_cmp expected actual
 '
 
+test_expect_success 'line with leading whitespace is not trailer' '
+   q_to_tab >patch <<-\EOF &&
+
+   Qtoken: value
+   EOF
+   q_to_tab >expected <<-\EOF &&
+
+   Qtoken: value
+
+   token: value
+   EOF
+   git interpret-trailers --trailer "token: value" patch >actual &&
+   test_cmp expected actual
+'
+
 test_expect_success 'with config setup' '
git config trailer.ack.key "Acked-by: " &&
cat >expected <<-\EOF &&
diff --git a/trailer.c b/trailer.c
index 199f86a..d978437 100644
--- a/trailer.c
+++ b/trailer.c
@@ -775,7 +775,7 @@ static int find_trailer_start(struct strbuf **lines, int 
count)
}
 
separator_pos = find_separator(lines[start]->buf);
-   if (separator_pos >= 1) {
+   if (separator_pos >= 1 && !isspace(lines[start]->buf[0])) {
struct list_head *pos;
 
trailer_lines++;
-- 
2.8.0.rc3.226.g39d4020

[PATCH v5 6/8] trailer: allow non-trailers in trailer block

[Jonathan Tan]

Currently, interpret-trailers requires all lines of a trailer block to
be trailers (or comments) - if not it would not identify that block as a
trailer block, and thus create its own trailer block, inserting a blank
line.  For example:

  echo -e "\nSigned-off-by: x\nnot trailer" |
  git interpret-trailers --trailer "c: d"

would result in:

  Signed-off-by: x
  not trailer

  c: d

Relax the definition of a trailer block to require that the trailers (i)
are all trailers, or (ii) contain at least one Git-generated trailer and
consists of at least 25% trailers.

  Signed-off-by: x
  not trailer
  c: d

(i) is the existing functionality. (ii) allows arbitrary lines to be
included in trailer blocks, like those in [1], and still allow
interpret-trailers to be used.

[1]
https://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/e7d316a02f683864a12389f8808570e37fb90aa3

Signed-off-by: Jonathan Tan 
---
 Documentation/git-interpret-trailers.txt |   5 +-
 t/t7513-interpret-trailers.sh| 115 +++
 trailer.c|  89 
 3 files changed, 194 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index 93d1db6..cf4c5ea 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -48,8 +48,9 @@ with only spaces at the end of the commit message part, one 
blank line
 will be added before the new trailer.
 
 Existing trailers are extracted from the input message by looking for
-a group of one or more lines that contain a colon (by default), where
-the group is preceded by one or more empty (or whitespace-only) lines.
+a group of one or more lines that (i) are all trailers, or (ii) contains at
+least one Git-generated trailer and consists of at least 25% trailers.
+The group must be preceded by one or more empty (or whitespace-only) lines.
 The group must either be at the end of the message or be the last
 non-whitespace lines before a line that starts with '---'. Such three
 minus signs start the patch part of the message.
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index aee785c..003e90f 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -126,6 +126,121 @@ test_expect_success 'with multiline title in the message' 
'
test_cmp expected actual
 '
 
+test_expect_success 'with non-trailer lines mixed with Signed-off-by' '
+   cat >patch <<-\EOF &&
+
+   this is not a trailer
+   this is not a trailer
+   Signed-off-by: a 
+   this is not a trailer
+   EOF
+   cat >expected <<-\EOF &&
+
+   this is not a trailer
+   this is not a trailer
+   Signed-off-by: a 
+   this is not a trailer
+   token: value
+   EOF
+   git interpret-trailers --trailer "token: value" patch >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'with non-trailer lines mixed with cherry picked from' '
+   cat >patch <<-\EOF &&
+
+   this is not a trailer
+   this is not a trailer
+   (cherry picked from commit x)
+   this is not a trailer
+   EOF
+   cat >expected <<-\EOF &&
+
+   this is not a trailer
+   this is not a trailer
+   (cherry picked from commit x)
+   this is not a trailer
+   token: value
+   EOF
+   git interpret-trailers --trailer "token: value" patch >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'with non-trailer lines mixed with a configured trailer' '
+   cat >patch <<-\EOF &&
+
+   this is not a trailer
+   this is not a trailer
+   My-trailer: x
+   this is not a trailer
+   EOF
+   cat >expected <<-\EOF &&
+
+   this is not a trailer
+   this is not a trailer
+   My-trailer: x
+   this is not a trailer
+   token: value
+   EOF
+   test_config trailer.my.key "My-trailer: " &&
+   git interpret-trailers --trailer "token: value" patch >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'with non-trailer lines mixed with a non-configured 
trailer' '
+   cat >patch <<-\EOF &&
+
+   this is not a trailer
+   this is not a trailer
+   I-am-not-configured: x
+   this is not a trailer
+   EOF
+   cat >expected <<-\EOF &&
+
+   this is not a trailer
+   this is not a trailer
+   I-am-not-configured: x
+   this is not a trailer
+
+   token: value
+   EOF
+   test_config trailer.my.key "My-trailer: " &&
+   git 

[PATCH v5 0/8] allow non-trailers and multiple-line trailers

[Jonathan Tan]

I've updated patch 5/8 to use strcspn and to pass in the list of
separators, meaning that we no longer accept '=' in file input (and also
updated its commit message accordingly).

We also discussed inlining find_separator, but after looking at the
code, I think that it is more convenient if find_separator returns -1
when there is no separator, because of the 3 times it is used (as
of 8/8), it is checked twice with '>= 1' (since both "no separator" and
"string begins with separator" are handled in the same way - treating
them as a non-trailer line). So I have left it as its own function.

No other updates.

Jonathan Tan (8):
  trailer: improve const correctness
  trailer: use list.h for doubly-linked list
  trailer: streamline trailer item create and add
  trailer: make args have their own struct
  trailer: clarify failure modes in parse_trailer
  trailer: allow non-trailers in trailer block
  trailer: forbid leading whitespace in trailers
  trailer: support values folded to multiple lines

 Documentation/git-interpret-trailers.txt |  14 +-
 t/t7513-interpret-trailers.sh| 299 +++
 trailer.c| 620 +--
 3 files changed, 654 insertions(+), 279 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH v5 1/8] trailer: improve const correctness

[Jonathan Tan]

Change "const char *" to "char *" in struct trailer_item and in the
return value of apply_command (since those strings are owned strings).

Change "struct conf_info *" to "const struct conf_info *" (since that
struct is not modified).

Signed-off-by: Jonathan Tan 
---
 trailer.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/trailer.c b/trailer.c
index c6ea9ac..1f191b2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -27,8 +27,8 @@ static struct conf_info default_conf_info;
 struct trailer_item {
struct trailer_item *previous;
struct trailer_item *next;
-   const char *token;
-   const char *value;
+   char *token;
+   char *value;
struct conf_info conf;
 };
 
@@ -95,8 +95,8 @@ static void free_trailer_item(struct trailer_item *item)
free(item->conf.name);
free(item->conf.key);
free(item->conf.command);
-   free((char *)item->token);
-   free((char *)item->value);
+   free(item->token);
+   free(item->value);
free(item);
 }
 
@@ -215,13 +215,13 @@ static struct trailer_item *remove_first(struct 
trailer_item **first)
return item;
 }
 
-static const char *apply_command(const char *command, const char *arg)
+static char *apply_command(const char *command, const char *arg)
 {
struct strbuf cmd = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
struct child_process cp = CHILD_PROCESS_INIT;
const char *argv[] = {NULL, NULL};
-   const char *result;
+   char *result;
 
strbuf_addstr(, command);
if (arg)
@@ -425,7 +425,7 @@ static int set_if_missing(struct conf_info *item, const 
char *value)
return 0;
 }
 
-static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
+static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
 {
*dst = *src;
if (src->name)
-- 
2.8.0.rc3.226.g39d4020

[PATCH v5 5/8] trailer: clarify failure modes in parse_trailer

[Jonathan Tan]

The parse_trailer function has a few modes of operation, all depending
on whether the separator is present in its input, and if yes, the
separator's position. Some of these modes are failure modes, and these
failure modes are handled differently depending on whether the trailer
line was sourced from a file or from a command-line argument.

Extract a function to find the separator, allowing the invokers of
parse_trailer to determine how to handle the failure modes instead of
making parse_trailer do it. In this function, also take in the list of
separators, so that we can distinguish between command line arguments
(which allow '=' as separator) and file input (which does not allow '='
as separator).

Signed-off-by: Jonathan Tan 
---
 trailer.c | 75 ---
 1 file changed, 53 insertions(+), 22 deletions(-)

diff --git a/trailer.c b/trailer.c
index 99018f8..aff858b 100644
--- a/trailer.c
+++ b/trailer.c
@@ -543,29 +543,37 @@ static int token_matches_item(const char *tok, struct 
arg_item *item, int tok_le
return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
 }
 
-static int parse_trailer(struct strbuf *tok, struct strbuf *val,
-const struct conf_info **conf, const char *trailer)
+/*
+ * Return the location of the first separator in line, or -1 if there is no
+ * separator.
+ */
+static int find_separator(const char *line, const char *separators)
+{
+   int loc = strcspn(line, separators);
+   if (!line[loc])
+   return -1;
+   return loc;
+}
+
+/*
+ * Obtain the token, value, and conf from the given trailer.
+ *
+ * separator_pos must not be 0, since the token cannot be an empty string.
+ *
+ * If separator_pos is -1, interpret the whole trailer as a token.
+ */
+static void parse_trailer(struct strbuf *tok, struct strbuf *val,
+const struct conf_info **conf, const char *trailer,
+int separator_pos)
 {
-   size_t len;
-   struct strbuf seps = STRBUF_INIT;
struct arg_item *item;
int tok_len;
struct list_head *pos;
 
-   strbuf_addstr(, separators);
-   strbuf_addch(, '=');
-   len = strcspn(trailer, seps.buf);
-   strbuf_release();
-   if (len == 0) {
-   int l = strlen(trailer);
-   while (l > 0 && isspace(trailer[l - 1]))
-   l--;
-   return error(_("empty trailer token in trailer '%.*s'"), l, 
trailer);
-   }
-   if (len < strlen(trailer)) {
-   strbuf_add(tok, trailer, len);
+   if (separator_pos != -1) {
+   strbuf_add(tok, trailer, separator_pos);
strbuf_trim(tok);
-   strbuf_addstr(val, trailer + len + 1);
+   strbuf_addstr(val, trailer + separator_pos + 1);
strbuf_trim(val);
} else {
strbuf_addstr(tok, trailer);
@@ -587,8 +595,6 @@ static int parse_trailer(struct strbuf *tok, struct strbuf 
*val,
break;
}
}
-
-   return 0;
 }
 
 static void add_trailer_item(struct list_head *head, char *tok, char *val)
@@ -619,6 +625,12 @@ static void process_command_line_args(struct list_head 
*arg_head,
const struct conf_info *conf;
struct list_head *pos;
 
+   /*
+* In command-line arguments, '=' is accepted (in addition to the
+* separators that are defined).
+*/
+   char *cl_separators = xstrfmt("=%s", separators);
+
/* Add an arg item for each configured trailer with a command */
list_for_each(pos, _head) {
item = list_entry(pos, struct arg_item, list);
@@ -631,12 +643,25 @@ static void process_command_line_args(struct list_head 
*arg_head,
 
/* Add an arg item for each trailer on the command line */
for_each_string_list_item(tr, trailers) {
-   if (!parse_trailer(, , , tr->string))
+   int separator_pos = find_separator(tr->string, cl_separators);
+   if (separator_pos == 0) {
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_addstr(, tr->string);
+   strbuf_trim();
+   error(_("empty trailer token in trailer '%.*s'"),
+ (int) sb.len, sb.buf);
+   strbuf_release();
+   } else {
+   parse_trailer(, , , tr->string,
+ separator_pos);
add_arg_item(arg_head,
 strbuf_detach(, NULL),
 strbuf_detach(, NULL),
 conf);
+   }
}
+
+   free(cl_separators);
 }
 
 static struct strbuf **read_input_file(const char *file)
@@ -775,11 +800,17 @@ static int process_input_file(FILE 

[PATCH v5 4/8] trailer: make args have their own struct

[Jonathan Tan]

Improve type safety by making arguments (whether from configuration or
from the command line) have their own "struct arg_item" type, separate
from the "struct trailer_item" type used to represent the trailers in
the buffer being manipulated.

This change also prepares "struct trailer_item" to be further
differentiated from "struct arg_item" in future patches.

Signed-off-by: Jonathan Tan 
---
 trailer.c | 135 +++---
 1 file changed, 85 insertions(+), 50 deletions(-)

diff --git a/trailer.c b/trailer.c
index ae3972a..99018f8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -29,6 +29,12 @@ struct trailer_item {
struct list_head list;
char *token;
char *value;
+};
+
+struct arg_item {
+   struct list_head list;
+   char *token;
+   char *value;
struct conf_info conf;
 };
 
@@ -62,7 +68,7 @@ static size_t token_len_without_separator(const char *token, 
size_t len)
return len;
 }
 
-static int same_token(struct trailer_item *a, struct trailer_item *b)
+static int same_token(struct trailer_item *a, struct arg_item *b)
 {
size_t a_len = token_len_without_separator(a->token, strlen(a->token));
size_t b_len = token_len_without_separator(b->token, strlen(b->token));
@@ -71,12 +77,12 @@ static int same_token(struct trailer_item *a, struct 
trailer_item *b)
return !strncasecmp(a->token, b->token, min_len);
 }
 
-static int same_value(struct trailer_item *a, struct trailer_item *b)
+static int same_value(struct trailer_item *a, struct arg_item *b)
 {
return !strcasecmp(a->value, b->value);
 }
 
-static int same_trailer(struct trailer_item *a, struct trailer_item *b)
+static int same_trailer(struct trailer_item *a, struct arg_item *b)
 {
return same_token(a, b) && same_value(a, b);
 }
@@ -98,6 +104,13 @@ static inline void strbuf_replace(struct strbuf *sb, const 
char *a, const char *
 
 static void free_trailer_item(struct trailer_item *item)
 {
+   free(item->token);
+   free(item->value);
+   free(item);
+}
+
+static void free_arg_item(struct arg_item *item)
+{
free(item->conf.name);
free(item->conf.key);
free(item->conf.command);
@@ -137,17 +150,29 @@ static void print_all(FILE *outfile, struct list_head 
*head, int trim_empty)
}
 }
 
+static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
+{
+   struct trailer_item *new = xcalloc(sizeof(*new), 1);
+   new->token = arg_tok->token;
+   new->value = arg_tok->value;
+   arg_tok->token = arg_tok->value = NULL;
+   free_arg_item(arg_tok);
+   return new;
+}
+
 static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct trailer_item *arg_tok)
+ struct arg_item *arg_tok)
 {
-   if (after_or_end(arg_tok->conf.where))
-   list_add(_tok->list, _tok->list);
+   int aoe = after_or_end(arg_tok->conf.where);
+   struct trailer_item *to_add = trailer_from_arg(arg_tok);
+   if (aoe)
+   list_add(_add->list, _tok->list);
else
-   list_add_tail(_tok->list, _tok->list);
+   list_add_tail(_add->list, _tok->list);
 }
 
 static int check_if_different(struct trailer_item *in_tok,
- struct trailer_item *arg_tok,
+ struct arg_item *arg_tok,
  int check_all,
  struct list_head *head)
 {
@@ -200,7 +225,7 @@ static char *apply_command(const char *command, const char 
*arg)
return result;
 }
 
-static void apply_item_command(struct trailer_item *in_tok, struct 
trailer_item *arg_tok)
+static void apply_item_command(struct trailer_item *in_tok, struct arg_item 
*arg_tok)
 {
if (arg_tok->conf.command) {
const char *arg;
@@ -218,13 +243,13 @@ static void apply_item_command(struct trailer_item 
*in_tok, struct trailer_item
 }
 
 static void apply_arg_if_exists(struct trailer_item *in_tok,
-   struct trailer_item *arg_tok,
+   struct arg_item *arg_tok,
struct trailer_item *on_tok,
struct list_head *head)
 {
switch (arg_tok->conf.if_exists) {
case EXISTS_DO_NOTHING:
-   free_trailer_item(arg_tok);
+   free_arg_item(arg_tok);
break;
case EXISTS_REPLACE:
apply_item_command(in_tok, arg_tok);
@@ -241,39 +266,41 @@ static void apply_arg_if_exists(struct trailer_item 
*in_tok,
if (check_if_different(in_tok, arg_tok, 1, head))
add_arg_to_input_list(on_tok, arg_tok);
else
-   free_trailer_item(arg_tok);
+   free_arg_item(arg_tok);
break;
case 

Re: [BUG] [PATCH]: run-command.c

[Junio C Hamano]

Jeff King  writes:

>> 16:42:45$ cat /proc/2601/cmdline | xargs -0 -n1 echo
>> /bin/sh
>> -c
>> echo $PWD;sleep 600
>> echo $PWD;sleep 600
>
> This duplicated argument is expected and normal.

Well explained.  Thanks.

Re: [PATCH 0/3] fix travis TAP/--verbose conflict

[Stefan Beller]

On Fri, Oct 21, 2016 at 3:41 AM, Jeff King  wrote:
> On Fri, Oct 21, 2016 at 04:43:48AM -0400, Jeff King wrote:
>
>> The obvious fix would be to send "--verbose" output to stderr, but I
>> suspect that would end up annoying for people who do:
>>
>>   ./t5547-push-quarantine.sh -v | less
>>
>> to read long output. Probably we need some option like "--log" which
>> logs in the same way that "--tee" does, but _without_ sending the data
>> to stdout. Naively, that just means replacing the "tee" invocation with
>> "cat", but I suspect it will be a lot more complicated than that,
>> because we still need to let the TAP output go to stdout.
>
> Yeah, it was definitely a lot more complicated. This patch series fixes
> it.
>
>   [1/3]: test-lib: handle TEST_OUTPUT_DIRECTORY with spaces
>   [2/3]: test-lib: add --verbose-log option
>   [3/3]: travis: use --verbose-log test option

All patches look good to me
(1&3 are obvious, and 2 is very well described).

Thanks,
Stefan

Re: [PATCH 2/3] test-lib: add --verbose-log option

[Junio C Hamano]

Jeff King  writes:

> This patch implements option (5), which seems to work well
> in practice.

Your journey to reach this final design and implementation may have
been painful---I really like the end result.  This was an accident
waiting to happen.

Thanks for fixing it.

> @@ -319,7 +332,10 @@ fi
>  
>  exec 5>&1
>  exec 6<&0
> -if test "$verbose" = "t"
> +if test "$verbose_log" = "t"
> +then
> + exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
> +elif test "$verbose" = "t"
>  then
>   exec 4>&2 3>&1
>  else

OK, unlike "verbose" case where we give 3 (saved stdout) to 1 and 4
(saved stderr) to 2, we send 3 to the output file and 4 to the same.

Makes sense.

Re: [BUG] fetch output is ugly in 'next'

[Junio C Hamano]

Duy Nguyen  writes:

> On Fri, Oct 21, 2016 at 7:11 PM, Duy Nguyen  wrote:
>> Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something
>> sensible should do it.
>
> Correction (if somebody will pick this up), it's
> TRANSPORT_SUMMARY_WIDTH that needs to be adjusted, not those four.

Yes, it used to be and it still is (2 * DEFAULT_ABBREV + 3) but in
the new world order where default-abbrev is often -1 the expression
does not make much sense.

Perhaps something along this line?

 transport.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/transport.h b/transport.h
index 18d2cf8275..5339fabbad 100644
--- a/transport.h
+++ b/transport.h
@@ -127,7 +127,7 @@ struct transport {
 #define TRANSPORT_PUSH_CERT 2048
 #define TRANSPORT_PUSH_ATOMIC 4096
 
-#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
+#define TRANSPORT_SUMMARY_WIDTH (2 * (DEFAULT_ABBREV < 0 ? 7 : DEFAULT_ABBREV) 
+ 3)
 #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - 
gettext_width(x)), (x)
 
 /* Returns a transport suitable for the url */

Re: [PATCH 1/2] t9000-addresses: update expected results after fix

[Junio C Hamano]

Matthieu Moy  writes:

> e3fdbcc8e1 (parse_mailboxes: accept extra text after <...> address,
> 2016-10-13) improved our in-house address parser and made it closer to
> Mail::Address. As a consequence, some tests comparing it to
> Mail::Address now pass, but e3fdbcc8e1 forgot to update the test.
>
> Signed-off-by: Matthieu Moy 
> ---

Thanks.

Re: Problems with "git svn clone"

[K Richard Pixley]

Thank you.

Running on debian stretch/testing it got past the 11 second mark and has
been running for a day now.  I expect it will take days, perhaps longer,
so we'll see.

--rich



- CONFIDENTIAL-

This email and any files transmitted with it are confidential, and may also be 
legally privileged. If you are not the intended recipient, you may not review, 
use, copy, or distribute this message. If you receive this email in error, 
please notify the sender immediately by reply email and then delete this email.

Re: What's cooking in git.git (Oct 2016, #05; Thu, 20)

[Lars Schneider]

> On 21 Oct 2016, at 06:08, Johannes Schindelin  
> wrote:
> 
> Hi Junio & Lars,
> 
> On Thu, 20 Oct 2016, Junio C Hamano wrote:
> 
>> * ls/filter-process (2016-10-17) 14 commits
>>  (merged to 'next' on 2016-10-19 at ffd0de042c)
>> + contrib/long-running-filter: add long running filter example
>> + convert: add filter..process option
>> + convert: prepare filter..process option
>> + convert: make apply_filter() adhere to standard Git error handling
>> + pkt-line: add functions to read/write flush terminated packet streams
>> + pkt-line: add packet_write_gently()
>> + pkt-line: add packet_flush_gently()
>> + pkt-line: add packet_write_fmt_gently()
>> + pkt-line: extract set_packet_header()
>> + pkt-line: rename packet_write() to packet_write_fmt()
>> + run-command: add clean_on_exit_handler
>> + run-command: move check_pipe() from write_or_die to run_command
>> + convert: modernize tests
>> + convert: quote filter names in error messages
>> 
>> The smudge/clean filter API expect an external process is spawned
>> to filter the contents for each path that has a filter defined.  A
>> new type of "process" filter API has been added to allow the first
>> request to run the filter for a path to spawn a single process, and
>> all filtering need is served by this single process for multiple
>> paths, reducing the process creation overhead.
>> 
>> Will merge to 'master'.
> 
> This breaks in Git for Windows' SDK (I only now realized that t0060 was
> not the only thing breaking in `next` for a while now):
> ...
> -- snap --
> 
> Unsurprisingly, bisect identifies "convert: add filter..process
> option" as the first bad commit, although it only fails on the test case
> 15, but not on 17.
> 
> I am unfortunately still busy with trying to figure out what exactly makes
> t6030 hang on `pu` (seems it thinks stdin is a tty and just waits for an
> answer), and then trying to reduce that insane amount of time wasted on
> running, and waiting for, the test suite, and for unrelated reasons I'll
> have to go offline for the rest of the day, so I will most likely be
> unable to assist further with this.

Hi Johannes,

thanks for reporting this. Looks like I misunderstood your comment when
you wrote the error is already fixed in GfW:
https://github.com/git-for-windows/git/issues/770#issuecomment-251426487

I think this patch series (referenced by the link above, too) should fix 
the problem:
http://public-inbox.org/git/2016090521.72956-1-larsxschnei...@gmail.com/

Don't waste any time on this. I will look into it ASAP (traveling right now).

Cheers,
Lars

Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths

[Junio C Hamano]

Duy Nguyen  writes:

> The amount of changes is unbelievable for fixing such a rare case
> though. I wonder if we can just detect this in daemon.c and pass
> "./~foo/bar" instead of "~foo/bar" to enter_repo() in non-strict mode
> to "disable" expand_user_path(). If it works, it's much simpler
> changes (even though a bit hacky)

Conceptually, it ought to be updating the code that says "Does it
begin with a tilde?  Then try to do user-path expansion and fail if
that is not enabled and if it succeeds use the resulting directory"
to "Is user-path enabled and does it begin with a tilde?  Then try
to do user-path expansion and if it succeeds use the resulting
directory".  Compared to that mental model we have with this
codepath, I agree that the change does look quite invasive and
large.

It is OK for a change to be large, as long as the end result is
easier to read and understand than the original.  I am undecided if
that is the case with this patch, though.

Also I am a bit worried about the change in the semantics.  While it
is not the best way to achieve this, the server operators could use
it as a way to add directories whose contents need to be hidden to
give them names that begin with a tilde without enabling user-path
expansion.  This change may be a new and useful feature for Luke,
but to these server operators this change can be a new bug---it is
probably a minor new bug as they can work it around by using other
means to hide these directories, though.

Re: Prove "Tests out of sequence" Error

[Jacob Keller]

On Fri, Oct 21, 2016 at 8:48 AM, Jeff King  wrote:
> On Fri, Oct 21, 2016 at 08:42:58AM -0700, Jacob Keller wrote:
>
>> > For $HARNESS_ACTIVE with _just_ "--verbose", I don't think it would be a
>> > good idea to activate it. We should either silently ignore --verbose
>> > then, or complain and die.
>>
>> We should probably do that to make sure people realize what might
>> happen. I read your series and it has a good explanation of the
>> possible alternatives. I like the approach you chose.
>
> Thanks. Do you want to make a patch on top of my series?
>
> -Peff

I am not sure I will find time to do it today, so it wouldn't be for a
few more days.

Thanks,
Jake

Re: Prove "Tests out of sequence" Error

[Jeff King]

On Fri, Oct 21, 2016 at 08:42:58AM -0700, Jacob Keller wrote:

> > For $HARNESS_ACTIVE with _just_ "--verbose", I don't think it would be a
> > good idea to activate it. We should either silently ignore --verbose
> > then, or complain and die.
> 
> We should probably do that to make sure people realize what might
> happen. I read your series and it has a good explanation of the
> possible alternatives. I like the approach you chose.

Thanks. Do you want to make a patch on top of my series?

-Peff

Re: Prove "Tests out of sequence" Error

[Jacob Keller]

On Fri, Oct 21, 2016 at 8:35 AM, Jeff King  wrote:
> On Fri, Oct 21, 2016 at 08:29:49AM -0700, Jacob Keller wrote:
>
>> > The Travis tests do exactly this (along with --tee to actually save the
>> > output). It seems like a minor miracle that this is the first test
>> > output that has actually triggered as TAP input. I'd suggest that the
>> > problem is not in the test, though, but that our "--verbose" option is
>> > unsuitable for using with a TAP harness.
>> >
>> > The obvious fix would be to send "--verbose" output to stderr, but I
>> > suspect that would end up annoying for people who do:
>> >
>> >   ./t5547-push-quarantine.sh -v | less
>> >
>> > to read long output. Probably we need some option like "--log" which
>> > logs in the same way that "--tee" does, but _without_ sending the data
>> > to stdout. Naively, that just means replacing the "tee" invocation with
>> > "cat", but I suspect it will be a lot more complicated than that,
>> > because we still need to let the TAP output go to stdout.
>>
>> Can we determine that we're running with something monitoring the TAP
>> output? Because then we could make verbose go to stderr instead
>> dynamically?
>
> I think $HARNESS_ACTIVE could tell us that. But the hard part isn't
> activating it; it's directing the verbose output to the log without
> sending it to stdout.
>
> See the patch I posted later in the thread, and my musings on
> auto-activating it. I guess we could do so safely when we see
> $HARNESS_ACTIVE along with "--tee" and "--verbose", though I don't know
> if it's worth the trouble.
>
> For $HARNESS_ACTIVE with _just_ "--verbose", I don't think it would be a
> good idea to activate it. We should either silently ignore --verbose
> then, or complain and die.
>

We should probably do that to make sure people realize what might
happen. I read your series and it has a good explanation of the
possible alternatives. I like the approach you chose.

Thanks,
Jake

> -Peff

Re: [PATCH v4 20/25] sequencer: refactor write_message()

[Junio C Hamano]

Johannes Schindelin  writes:

>> Ah, make that four steps.  The final one is:
>> 
>> - add append_eol parameter that nobody uses at this step in the
>>   series.
>>
>> This is a new feature to the helper.  While it is OK to have it as a
>> preparatory step in this series, it is easier to understand if it
>> kept as a separate step.  It is even more preferrable if it is made
>> as a preparatory step in a series that adds a caller that passes
>> true to append_eol to this helper...
>
> Done,
> Dscho

Hmm, what has been done exactly?  I still see append_eol in v5 where
nobody uses it yet.  Confused...

Re: Prove "Tests out of sequence" Error

[Jeff King]

On Fri, Oct 21, 2016 at 08:29:49AM -0700, Jacob Keller wrote:

> > The Travis tests do exactly this (along with --tee to actually save the
> > output). It seems like a minor miracle that this is the first test
> > output that has actually triggered as TAP input. I'd suggest that the
> > problem is not in the test, though, but that our "--verbose" option is
> > unsuitable for using with a TAP harness.
> >
> > The obvious fix would be to send "--verbose" output to stderr, but I
> > suspect that would end up annoying for people who do:
> >
> >   ./t5547-push-quarantine.sh -v | less
> >
> > to read long output. Probably we need some option like "--log" which
> > logs in the same way that "--tee" does, but _without_ sending the data
> > to stdout. Naively, that just means replacing the "tee" invocation with
> > "cat", but I suspect it will be a lot more complicated than that,
> > because we still need to let the TAP output go to stdout.
> 
> Can we determine that we're running with something monitoring the TAP
> output? Because then we could make verbose go to stderr instead
> dynamically?

I think $HARNESS_ACTIVE could tell us that. But the hard part isn't
activating it; it's directing the verbose output to the log without
sending it to stdout.

See the patch I posted later in the thread, and my musings on
auto-activating it. I guess we could do so safely when we see
$HARNESS_ACTIVE along with "--tee" and "--verbose", though I don't know
if it's worth the trouble.

For $HARNESS_ACTIVE with _just_ "--verbose", I don't think it would be a
good idea to activate it. We should either silently ignore --verbose
then, or complain and die.

-Peff

Re: What's cooking in git.git (Oct 2016, #05; Thu, 20)

[Pranit Bauva]

Hey Johannes,

On Fri, Oct 21, 2016 at 6:38 PM, Johannes Schindelin
 wrote:
> I am unfortunately still busy with trying to figure out what exactly makes
> t6030 hang on `pu` (seems it thinks stdin is a tty and just waits for an
> answer), and then trying to reduce that insane amount of time wasted on
> running, and waiting for, the test suite, and for unrelated reasons I'll
> have to go offline for the rest of the day, so I will most likely be
> unable to assist further with this.

My patch series [1] is recently merged into pu which changed a lot of
things around "git bisect next" which I remember that you mention it
somewhere else too but don't exactly recollect where. There were some
*really* drastic changes and they are evident with my conversation
with Junio. Maybe you could tell a little bit more about what's
happening with t6030 so that even I could dig further.

Heads up: The changes in "git bisect next" were related to revision
walking so you might want to check it out.

Sorry for any inconvenience caused by this series.

Regards,
Pranit Bauva

Re: Prove "Tests out of sequence" Error

[Jacob Keller]

On Fri, Oct 21, 2016 at 1:43 AM, Jeff King  wrote:
>   prove t5547-push-quarantine.sh :: -v
>
> The Travis tests do exactly this (along with --tee to actually save the
> output). It seems like a minor miracle that this is the first test
> output that has actually triggered as TAP input. I'd suggest that the
> problem is not in the test, though, but that our "--verbose" option is
> unsuitable for using with a TAP harness.
>
> The obvious fix would be to send "--verbose" output to stderr, but I
> suspect that would end up annoying for people who do:
>
>   ./t5547-push-quarantine.sh -v | less
>
> to read long output. Probably we need some option like "--log" which
> logs in the same way that "--tee" does, but _without_ sending the data
> to stdout. Naively, that just means replacing the "tee" invocation with
> "cat", but I suspect it will be a lot more complicated than that,
> because we still need to let the TAP output go to stdout.
>
> -Peff

Can we determine that we're running with something monitoring the TAP
output? Because then we could make verbose go to stderr instead
dynamically?

Thanks,
Jake

Re: Drastic jump in the time required for the test suite

[Matthieu Moy]

René Scharfe  writes:

> I get this on WSL with prove -j8:
>
> Files=750, Tests=13657, 906 wallclock secs ( 8.51 usr 17.17 sys + 282.62 cusr 
> 3731.85 csys = 4040.15 CPU)
>
> And this for a run on Debian inside a Hyper-V VM on the same system:
>
> Files=759, Tests=13895, 99 wallclock secs ( 4.81 usr  1.06 sys + 39.70 cusr 
> 25.82 csys = 71.39 CPU)

What about the same without WSL on windows?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Git context menu in Windows Exporer has a “git bash” option but it doesn't work

[Stefan Monov]

On Fri, Oct 21, 2016 at 3:40 PM, Johannes Schindelin
 wrote:
> Are you referring to Git for Windows?
> Which version? From where did you obtain it?
I'm not sure about the answer to these questions, so to clear things
up, I'm downloading it anew now. Getting it from
https://git-scm.com/download/win .
I downloaded and installed Git for Windows, version 2.10.1, 32-bit.

Indeed the context menu option is now called "Git Bash Here". I don't
know what it was called before I installed this new version of git,
but now it works fine. I.e. no more auto-closing bash window.

Thanks for (indirectly) helping me resolve the issue. :)

On Fri, Oct 21, 2016 at 3:40 PM, Johannes Schindelin
 wrote:
> Hi Stefan,
>
> On Fri, 21 Oct 2016, Stefan Monov wrote:
>
>> The default git distribution for windows
>
> Are you referring to Git for Windows? Which version? From where did you
> obtain it?
>
>> contains, besides the CLI tools, a shell integration, i.e. a context
>> menu for Windows Explorer.  This context menu has a "Git bash" option
>> which I suppose should open a Git bash prompt cd'd to the current
>> directory.
>
> I guess you mean either "Git Bash Here" or "Git Bash" (not "Git bash")?
>
>> But instead, the git bash window opens and immediately closes, giving me
>> no chance to even see what it says inside it. Any fix?
>
> Sorry, there is not enough information to go on here. In Git for Windows'
> bug tracker, we were careful to provide you with a template that helps
> with providing relevant information:
> https://github.com/git-for-windows/git/issues/new
>
>> Note: Opening Git Bash from the Windows Start Menu works fine, but if
>> I open it that way, then I need to manually cd to the needed dir.
>
> For me, both work fine. For the record, I use Git for Windows (64-bit)
> v2.10.1, as downloaded from https://git-for-windows.github.io/:
>
> $ git version
> git version 2.10.1.windows.1
>
> Ciao,
> Johannes
>

Re: What's cooking in git.git (Oct 2016, #05; Thu, 20)

[Johannes Schindelin]

Hi Junio & Lars,

On Thu, 20 Oct 2016, Junio C Hamano wrote:

> * ls/filter-process (2016-10-17) 14 commits
>   (merged to 'next' on 2016-10-19 at ffd0de042c)
>  + contrib/long-running-filter: add long running filter example
>  + convert: add filter..process option
>  + convert: prepare filter..process option
>  + convert: make apply_filter() adhere to standard Git error handling
>  + pkt-line: add functions to read/write flush terminated packet streams
>  + pkt-line: add packet_write_gently()
>  + pkt-line: add packet_flush_gently()
>  + pkt-line: add packet_write_fmt_gently()
>  + pkt-line: extract set_packet_header()
>  + pkt-line: rename packet_write() to packet_write_fmt()
>  + run-command: add clean_on_exit_handler
>  + run-command: move check_pipe() from write_or_die to run_command
>  + convert: modernize tests
>  + convert: quote filter names in error messages
> 
>  The smudge/clean filter API expect an external process is spawned
>  to filter the contents for each path that has a filter defined.  A
>  new type of "process" filter API has been added to allow the first
>  request to run the filter for a path to spawn a single process, and
>  all filtering need is served by this single process for multiple
>  paths, reducing the process creation overhead.
> 
>  Will merge to 'master'.

This breaks in Git for Windows' SDK (I only now realized that t0060 was
not the only thing breaking in `next` for a while now):

-- snip --
not ok 15 - required process filter should filter data
#
#   test_config_global filter.protocol.process
#   "$TEST_DIRECTORY/t002
#   1/rot13-filter.pl clean smudge" &&
#   test_config_global filter.protocol.required true &&
#   rm -rf repo &&
#   mkdir repo &&
#   (
#   cd repo &&
#   git init &&
#
#   echo "git-stderr.log" >.gitignore &&
#   echo "*.r filter=protocol" >.gitattributes &&
#   git add . &&
#   git commit . -m "test commit 1" &&
#   git branch empty-branch &&
#
#   cp "$TEST_ROOT/test.o" test.r &&
#   cp "$TEST_ROOT/test2.o" test2.r &&
#   mkdir testsubdir &&
#   cp "$TEST_ROOT/test3 'sq',\$x.o" "testsubdir/test3
#   'sq',
#   \$x.r" &&
#   >test4-empty.r &&
#
#   S=$(file_size test.r) &&
#   S2=$(file_size test2.r) &&
#   S3=$(file_size "testsubdir/test3 'sq',\$x.r") &&
#
#   filter_git add . &&
#   cat >expected.log <<-EOF &&
#   START
#   init handshake complete
#   IN: clean test.r $S [OK] -- OUT: $S . [OK]
#   IN: clean test2.r $S2 [OK] -- OUT: $S2 .
#   [OK]
#   IN: clean test4-empty.r 0 [OK] -- OUT: 0
#   [OK]
#   IN: clean testsubdir/test3 'sq',\$x.r $S3
#   [OK] -
#   - OUT: $S3 . [OK]
#   STOP
#   EOF
#   test_cmp_count expected.log rot13-filter.log &&
#
#   filter_git commit . -m "test commit 2" &&
#   cat >expected.log <<-EOF &&
#   START
#   init handshake complete
#   IN: clean test.r $S [OK] -- OUT: $S . [OK]
#   IN: clean test2.r $S2 [OK] -- OUT: $S2 .
#   [OK]
#   IN: clean test4-empty.r 0 [OK] -- OUT: 0
#   [OK]
#   IN: clean testsubdir/test3 'sq',\$x.r $S3
#   [OK] -
#   - OUT: $S3 . [OK]
#   IN: clean test.r $S [OK] -- OUT: $S . [OK]
#   IN: clean test2.r $S2 [OK] -- OUT: $S2 .
#   [OK]
#   IN: clean test4-empty.r 0 [OK] -- OUT: 0
#   [OK]
#   IN: clean testsubdir/test3 'sq',\$x.r $S3
#   [OK] -
#   - OUT: $S3 . [OK]
#   STOP
#   EOF
#   test_cmp_count expected.log rot13-filter.log &&
#
#   rm -f test2.r "testsubdir/test3 'sq',\$x.r" &&
#
#   filter_git checkout --quiet --no-progress . &&
#   cat >expected.log <<-EOF &&
#   START
#   

Re: [BUG] fetch output is ugly in 'next'

[Duy Nguyen]

On Fri, Oct 21, 2016 at 7:11 PM, Duy Nguyen  wrote:
> Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something
> sensible should do it.

Correction (if somebody will pick this up), it's
TRANSPORT_SUMMARY_WIDTH that needs to be adjusted, not those four.
-- 
Duy

Re: Git context menu in Windows Exporer has a “git bash” option but it doesn't work

[Johannes Schindelin]

Hi Stefan,

On Fri, 21 Oct 2016, Stefan Monov wrote:

> The default git distribution for windows

Are you referring to Git for Windows? Which version? From where did you
obtain it?

> contains, besides the CLI tools, a shell integration, i.e. a context
> menu for Windows Explorer.  This context menu has a "Git bash" option
> which I suppose should open a Git bash prompt cd'd to the current
> directory.

I guess you mean either "Git Bash Here" or "Git Bash" (not "Git bash")?

> But instead, the git bash window opens and immediately closes, giving me
> no chance to even see what it says inside it. Any fix?

Sorry, there is not enough information to go on here. In Git for Windows'
bug tracker, we were careful to provide you with a template that helps
with providing relevant information:
https://github.com/git-for-windows/git/issues/new

> Note: Opening Git Bash from the Windows Start Menu works fine, but if
> I open it that way, then I need to manually cd to the needed dir.

For me, both work fine. For the record, I use Git for Windows (64-bit)
v2.10.1, as downloaded from https://git-for-windows.github.io/:

$ git version
git version 2.10.1.windows.1

Ciao,
Johannes

[PATCH v5 19/27] sequencer: stop releasing the strbuf in write_message()

[Johannes Schindelin]

Nothing in the name "write_message()" suggests that the function
releases the strbuf passed to it. So let's release the strbuf in the
caller instead.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index d74fdce..745c86f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -243,7 +243,6 @@ static int write_message(struct strbuf *msgbuf, const char 
*filename)
return error_errno(_("Could not lock '%s'"), filename);
if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
return error_errno(_("Could not write to %s"), filename);
-   strbuf_release(msgbuf);
if (commit_lock_file(_file) < 0)
return error(_("Error wrapping up %s."), filename);
 
@@ -759,6 +758,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
free_commit_list(common);
free_commit_list(remotes);
}
+   strbuf_release();
 
/*
 * If the merge was clean or if it failed due to conflict, we write
-- 
2.10.1.583.g721a9e0

[PATCH v5 21/27] sequencer: refactor write_message() to take a pointer/length

[Johannes Schindelin]

Previously, we required an strbuf. But that limits the use case too much.
In the upcoming patch series (for which the current patch series prepares
the sequencer), we will want to write content to a file for which we have
a pointer and a length, not an strbuf.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 9fced42..300952f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -234,14 +234,14 @@ static void print_advice(int show_hint, struct 
replay_opts *opts)
}
 }
 
-static int write_message(struct strbuf *msgbuf, const char *filename)
+static int write_message(const void *buf, size_t len, const char *filename)
 {
static struct lock_file msg_file;
 
int msg_fd = hold_lock_file_for_update(_file, filename, 0);
if (msg_fd < 0)
return error_errno(_("Could not lock '%s'"), filename);
-   if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) {
+   if (write_in_full(msg_fd, buf, len) < 0) {
rollback_lock_file(_file);
return error_errno(_("Could not write to '%s'"), filename);
}
@@ -747,12 +747,14 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
 head, , opts);
if (res < 0)
return res;
-   res |= write_message(, git_path_merge_msg());
+   res |= write_message(msgbuf.buf, msgbuf.len,
+git_path_merge_msg());
} else {
struct commit_list *common = NULL;
struct commit_list *remotes = NULL;
 
-   res = write_message(, git_path_merge_msg());
+   res = write_message(msgbuf.buf, msgbuf.len,
+   git_path_merge_msg());
 
commit_list_insert(base, );
commit_list_insert(next, );
-- 
2.10.1.583.g721a9e0

[PATCH v5 20/27] sequencer: roll back lock file if write_message() failed

[Johannes Schindelin]

There is no need to wait until the atexit() handler kicks in at the end.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 745c86f..9fced42 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -241,10 +241,14 @@ static int write_message(struct strbuf *msgbuf, const 
char *filename)
int msg_fd = hold_lock_file_for_update(_file, filename, 0);
if (msg_fd < 0)
return error_errno(_("Could not lock '%s'"), filename);
-   if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
-   return error_errno(_("Could not write to %s"), filename);
-   if (commit_lock_file(_file) < 0)
+   if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) {
+   rollback_lock_file(_file);
+   return error_errno(_("Could not write to '%s'"), filename);
+   }
+   if (commit_lock_file(_file) < 0) {
+   rollback_lock_file(_file);
return error(_("Error wrapping up %s."), filename);
+   }
 
return 0;
 }
-- 
2.10.1.583.g721a9e0

[PATCH v5 23/27] sequencer: remove overzealous assumption in rebase -i mode

[Johannes Schindelin]

The sequencer was introduced to make the cherry-pick and revert
functionality available as library function, with the original idea
being to extend the sequencer to also implement the rebase -i
functionality.

The test to ensure that all of the commands in the script are identical
to the overall operation does not mesh well with that.

Therefore let's disable the test in rebase -i mode.

While at it, error out early if the "instruction sheet" (i.e. the todo
script) could not be parsed.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 000ce3e..bd11db4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -962,7 +962,10 @@ static int read_populate_todo(struct todo_list *todo_list,
close(fd);
 
res = parse_insn_buffer(todo_list->buf.buf, todo_list);
-   if (!res) {
+   if (res)
+   return error(_("Unusable instruction sheet: %s"), todo_file);
+
+   if (!is_rebase_i(opts)) {
enum todo_command valid =
opts->action == REPLAY_PICK ? TODO_PICK : TODO_REVERT;
int i;
@@ -976,8 +979,6 @@ static int read_populate_todo(struct todo_list *todo_list,
return error(_("Cannot revert during a 
cherry-pick."));
}
 
-   if (res)
-   return error(_("Unusable instruction sheet: %s"), todo_file);
return 0;
 }
 
-- 
2.10.1.583.g721a9e0

[PATCH v5 24/27] sequencer: mark action_name() for translation

[Johannes Schindelin]

The definition of this function goes back all the way to 043a449
(sequencer: factor code out of revert builtin, 2012-01-11), long before a
serious effort was made to translate all the error messages.

It is slightly out of the context of the current patch series (whose
purpose it is to re-implement the performance critical parts of the
interactive rebase in C) to make the error messages in the sequencer
translatable, but what the heck. We'll just do it while we're looking at
this part of the code.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index bd11db4..ff76b6f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -168,7 +168,7 @@ int sequencer_remove_state(struct replay_opts *opts)
 
 static const char *action_name(const struct replay_opts *opts)
 {
-   return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
+   return opts->action == REPLAY_REVERT ? N_("revert") : N_("cherry-pick");
 }
 
 struct commit_message {
@@ -300,10 +300,10 @@ static struct tree *empty_tree(void)
 static int error_dirty_index(struct replay_opts *opts)
 {
if (read_cache_unmerged())
-   return error_resolve_conflict(action_name(opts));
+   return error_resolve_conflict(_(action_name(opts)));
 
error(_("Your local changes would be overwritten by %s."),
-   action_name(opts));
+   _(action_name(opts)));
 
if (advice_commit_before_merge)
advise(_("Commit your changes or stash them to proceed."));
@@ -321,7 +321,7 @@ static int fast_forward_to(const unsigned char *to, const 
unsigned char *from,
if (checkout_fast_forward(from, to, 1))
return -1; /* the callee should have complained already */
 
-   strbuf_addf(, _("%s: fast-forward"), action_name(opts));
+   strbuf_addf(, _("%s: fast-forward"), _(action_name(opts)));
 
transaction = ref_transaction_begin();
if (!transaction ||
@@ -397,7 +397,7 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
write_locked_index(_index, _lock, COMMIT_LOCK))
/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
return error(_("%s: Unable to write new index file"),
-   action_name(opts));
+   _(action_name(opts)));
rollback_lock_file(_lock);
 
if (opts->signoff)
@@ -835,14 +835,14 @@ static int read_and_refresh_cache(struct replay_opts 
*opts)
if (read_index_preload(_index, NULL) < 0) {
rollback_lock_file(_lock);
return error(_("git %s: failed to read the index"),
-   action_name(opts));
+   _(action_name(opts)));
}
refresh_index(_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, 
NULL);
if (the_index.cache_changed && index_fd >= 0) {
if (write_locked_index(_index, _lock, COMMIT_LOCK)) {
rollback_lock_file(_lock);
return error(_("git %s: failed to refresh the index"),
-   action_name(opts));
+   _(action_name(opts)));
}
}
rollback_lock_file(_lock);
-- 
2.10.1.583.g721a9e0

[PATCH v5 25/27] sequencer: quote filenames in error messages

[Johannes Schindelin]

This makes the code consistent by fixing quite a couple of error messages.

Suggested by Jakub Narębski.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ff76b6f..340d0ed 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -252,7 +252,7 @@ static int write_message(const void *buf, size_t len, const 
char *filename,
}
if (commit_lock_file(_file) < 0) {
rollback_lock_file(_file);
-   return error(_("Error wrapping up %s."), filename);
+   return error(_("Error wrapping up '%s'."), filename);
}
 
return 0;
@@ -954,16 +954,16 @@ static int read_populate_todo(struct todo_list *todo_list,
strbuf_reset(_list->buf);
fd = open(todo_file, O_RDONLY);
if (fd < 0)
-   return error_errno(_("Could not open %s"), todo_file);
+   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);
+   return error(_("Could not read '%s'."), todo_file);
}
close(fd);
 
res = parse_insn_buffer(todo_list->buf.buf, todo_list);
if (res)
-   return error(_("Unusable instruction sheet: %s"), todo_file);
+   return error(_("Unusable instruction sheet: '%s'"), todo_file);
 
if (!is_rebase_i(opts)) {
enum todo_command valid =
@@ -1054,7 +1054,7 @@ static int read_populate_opts(struct replay_opts *opts)
 * are pretty certain that it is syntactically correct.
 */
if (git_config_from_file(populate_opts_cb, git_path_opts_file(), opts) 
< 0)
-   return error(_("Malformed options sheet: %s"),
+   return error(_("Malformed options sheet: '%s'"),
git_path_opts_file());
return 0;
 }
@@ -1097,7 +1097,7 @@ static int create_seq_dir(void)
return -1;
}
else if (mkdir(git_path_seq_dir(), 0777) < 0)
-   return error_errno(_("Could not create sequencer directory %s"),
+   return error_errno(_("Could not create sequencer directory 
'%s'"),
   git_path_seq_dir());
return 0;
 }
@@ -1116,12 +1116,12 @@ static int save_head(const char *head)
strbuf_addf(, "%s\n", head);
if (write_in_full(fd, buf.buf, buf.len) < 0) {
rollback_lock_file(_lock);
-   return error_errno(_("Could not write to %s"),
+   return error_errno(_("Could not write to '%s'"),
   git_path_head_file());
}
if (commit_lock_file(_lock) < 0) {
rollback_lock_file(_lock);
-   return error(_("Error wrapping up %s."), git_path_head_file());
+   return error(_("Error wrapping up '%s'."), 
git_path_head_file());
}
return 0;
 }
@@ -1166,9 +1166,9 @@ int sequencer_rollback(struct replay_opts *opts)
return rollback_single_pick();
}
if (!f)
-   return error_errno(_("cannot open %s"), git_path_head_file());
+   return error_errno(_("cannot open '%s'"), git_path_head_file());
if (strbuf_getline_lf(, f)) {
-   error(_("cannot read %s: %s"), git_path_head_file(),
+   error(_("cannot read '%s': %s"), git_path_head_file(),
  ferror(f) ?  strerror(errno) : _("unexpected end of 
file"));
fclose(f);
goto fail;
@@ -1207,7 +1207,7 @@ static int save_todo(struct todo_list *todo_list, struct 
replay_opts *opts)
todo_list->buf.len - offset) < 0)
return error_errno(_("Could not write to '%s'"), todo_path);
if (commit_lock_file(_lock) < 0)
-   return error(_("Error wrapping up %s."), todo_path);
+   return error(_("Error wrapping up '%s'."), todo_path);
return 0;
 }
 
-- 
2.10.1.583.g721a9e0

[PATCH v5 26/27] sequencer: start error messages consistently with lower case

[Johannes Schindelin]

Quite a few error messages touched by this developer during the work to
speed up rebase -i started with an upper case letter, violating our
current conventions. Instead of sneaking in this fix (and forgetting
quite a few error messages), let's just have one wholesale patch fixing
all of the error messages in the sequencer.

While at it, the funny "error: Error wrapping up..." was changed to a
less funny, but more helpful, "error: failed to finalize...".

Pointed out by Junio Hamano.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c   | 68 +--
 t/t3501-revert-cherry-pick.sh |  2 +-
 2 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 340d0ed..4c10c93 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -241,18 +241,18 @@ static int write_message(const void *buf, size_t len, 
const char *filename,
 
int msg_fd = hold_lock_file_for_update(_file, filename, 0);
if (msg_fd < 0)
-   return error_errno(_("Could not lock '%s'"), filename);
+   return error_errno(_("could not lock '%s'"), filename);
if (write_in_full(msg_fd, buf, len) < 0) {
rollback_lock_file(_file);
-   return error_errno(_("Could not write to '%s'"), filename);
+   return error_errno(_("could not write to '%s'"), filename);
}
if (append_eol && write(msg_fd, "\n", 1) < 0) {
rollback_lock_file(_file);
-   return error_errno(_("Could not write eol to '%s"), filename);
+   return error_errno(_("could not write eol to '%s"), filename);
}
if (commit_lock_file(_file) < 0) {
rollback_lock_file(_file);
-   return error(_("Error wrapping up '%s'."), filename);
+   return error(_("failed to finalize '%s'."), filename);
}
 
return 0;
@@ -302,11 +302,11 @@ static int error_dirty_index(struct replay_opts *opts)
if (read_cache_unmerged())
return error_resolve_conflict(_(action_name(opts)));
 
-   error(_("Your local changes would be overwritten by %s."),
+   error(_("your local changes would be overwritten by %s."),
_(action_name(opts)));
 
if (advice_commit_before_merge)
-   advise(_("Commit your changes or stash them to proceed."));
+   advise(_("commit your changes or stash them to proceed."));
return -1;
 }
 
@@ -415,7 +415,7 @@ static int is_index_unchanged(void)
struct commit *head_commit;
 
if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, head_sha1, NULL))
-   return error(_("Could not resolve HEAD commit\n"));
+   return error(_("could not resolve HEAD commit\n"));
 
head_commit = lookup_commit(head_sha1);
 
@@ -435,7 +435,7 @@ static int is_index_unchanged(void)
 
if (!cache_tree_fully_valid(active_cache_tree))
if (cache_tree_update(_index, 0))
-   return error(_("Unable to update cache tree\n"));
+   return error(_("unable to update cache tree\n"));
 
return !hashcmp(active_cache_tree->sha1, 
head_commit->tree->object.oid.hash);
 }
@@ -505,7 +505,7 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
if (!env) {
const char *gpg_opt = gpg_sign_opt_quoted(opts);
 
-   return error("You have staged changes in your working "
+   return error("you have staged changes in your working "
"tree. If these changes are meant to be\n"
"squashed into the previous commit, run:\n\n"
"  git commit --amend %s\n\n"
@@ -558,12 +558,12 @@ static int is_original_commit_empty(struct commit *commit)
const unsigned char *ptree_sha1;
 
if (parse_commit(commit))
-   return error(_("Could not parse commit %s\n"),
+   return error(_("could not parse commit %s\n"),
 oid_to_hex(>object.oid));
if (commit->parents) {
struct commit *parent = commit->parents->item;
if (parse_commit(parent))
-   return error(_("Could not parse parent commit %s\n"),
+   return error(_("could not parse parent commit %s\n"),
oid_to_hex(>object.oid));
ptree_sha1 = parent->tree->object.oid.hash;
} else {
@@ -647,7 +647,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
 * to work on.
 */
if (write_cache_as_tree(head, 0, NULL))
-   return error(_("Your index file is unmerged."));
+   return error(_("your index file is unmerged."));
} else {
 

[PATCH v5 22/27] sequencer: teach write_message() to append an optional LF

[Johannes Schindelin]

This commit prepares for future callers that will have a pointer/length
to some text to be written that lacks an LF, yet an LF is desired.
Instead of requiring the caller to append an LF to the buffer (and
potentially allocate memory to do so), the write_message() function
learns to append an LF at the end of the file.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 300952f..000ce3e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -234,7 +234,8 @@ static void print_advice(int show_hint, struct replay_opts 
*opts)
}
 }
 
-static int write_message(const void *buf, size_t len, const char *filename)
+static int write_message(const void *buf, size_t len, const char *filename,
+int append_eol)
 {
static struct lock_file msg_file;
 
@@ -245,6 +246,10 @@ static int write_message(const void *buf, size_t len, 
const char *filename)
rollback_lock_file(_file);
return error_errno(_("Could not write to '%s'"), filename);
}
+   if (append_eol && write(msg_fd, "\n", 1) < 0) {
+   rollback_lock_file(_file);
+   return error_errno(_("Could not write eol to '%s"), filename);
+   }
if (commit_lock_file(_file) < 0) {
rollback_lock_file(_file);
return error(_("Error wrapping up %s."), filename);
@@ -748,13 +753,13 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
if (res < 0)
return res;
res |= write_message(msgbuf.buf, msgbuf.len,
-git_path_merge_msg());
+git_path_merge_msg(), 0);
} else {
struct commit_list *common = NULL;
struct commit_list *remotes = NULL;
 
res = write_message(msgbuf.buf, msgbuf.len,
-   git_path_merge_msg());
+   git_path_merge_msg(), 0);
 
commit_list_insert(base, );
commit_list_insert(next, );
-- 
2.10.1.583.g721a9e0

[PATCH v5 27/27] sequencer: mark all error messages for translation

[Johannes Schindelin]

There was actually only one error message that was not yet marked for
translation.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 26 --
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 4c10c93..a61fe76 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -479,6 +479,20 @@ static char **read_author_script(void)
return env;
 }
 
+static const char staged_changes_advice[] =
+N_("you have staged changes in your working tree\n"
+"If these changes are meant to be squashed into the previous commit, run:\n"
+"\n"
+"  git commit --amend %s\n"
+"\n"
+"If they are meant to go into a new commit, run:\n"
+"\n"
+"  git commit %s\n"
+"\n"
+"In both cases, once you're done, continue with:\n"
+"\n"
+"  git rebase --continue\n");
+
 /*
  * If we are cherry-pick, and if the merge did not result in
  * hand-editing, we will hit this commit and inherit the original
@@ -505,16 +519,8 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
if (!env) {
const char *gpg_opt = gpg_sign_opt_quoted(opts);
 
-   return error("you have staged changes in your working "
-   "tree. If these changes are meant to be\n"
-   "squashed into the previous commit, run:\n\n"
-   "  git commit --amend %s\n\n"
-   "If they are meant to go into a new commit, "
-   "run:\n\n"
-   "  git commit %s\n\n"
-   "In both cases, once you're done, continue "
-   "with:\n\n"
-   "  git rebase --continue\n", gpg_opt, gpg_opt);
+   return error(_(staged_changes_advice),
+gpg_opt, gpg_opt);
}
}
 
-- 
2.10.1.583.g721a9e0

[PATCH v5 03/27] sequencer: avoid unnecessary indirection

[Johannes Schindelin]

We really do not need the *pointer to a* pointer to the options in
the read_populate_opts() function.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index cb16cbd..c2fbf6f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -813,7 +813,7 @@ static int populate_opts_cb(const char *key, const char 
*value, void *data)
return 0;
 }
 
-static int read_populate_opts(struct replay_opts **opts)
+static int read_populate_opts(struct replay_opts *opts)
 {
if (!file_exists(git_path_opts_file()))
return 0;
@@ -823,7 +823,7 @@ static int read_populate_opts(struct replay_opts **opts)
 * about this case, though, because we wrote that file ourselves, so we
 * are pretty certain that it is syntactically correct.
 */
-   if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) 
< 0)
+   if (git_config_from_file(populate_opts_cb, git_path_opts_file(), opts) 
< 0)
return error(_("Malformed options sheet: %s"),
git_path_opts_file());
return 0;
@@ -1054,7 +1054,7 @@ static int sequencer_continue(struct replay_opts *opts)
 
if (!file_exists(git_path_todo_file()))
return continue_single_pick();
-   if (read_populate_opts() ||
+   if (read_populate_opts(opts) ||
read_populate_todo(_list, opts))
return -1;
 
-- 
2.10.1.583.g721a9e0

[PATCH v5 13/27] sequencer: prepare for rebase -i's commit functionality

[Johannes Schindelin]

In interactive rebases, we commit a little bit differently than the
sequencer did so far: we heed the "author-script", the "message" and the
"amend" files in the .git/rebase-merge/ subdirectory.

Likewise, we may want to edit the commit message *even* when providing a
file containing the suggested commit message. Therefore we change the
code to not even provide a default message when we do not want any, and
to call the editor explicitly.

Also, in "interactive rebase" mode we want to skip reading the options
in the state directory of the cherry-pick/revert commands.

Finally, as interactive rebase's GPG settings are configured differently
from how cherry-pick (and therefore sequencer) handles them, we will
leave support for that to the next commit.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 99 ++---
 1 file changed, 89 insertions(+), 10 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3d1fdac..6d5fe94 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,6 +27,19 @@ static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
 static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
 static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
 
+/*
+ * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
+ * GIT_AUTHOR_DATE that will be used for the commit that is currently
+ * being rebased.
+ */
+static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
+
+/* We will introduce the 'interactive rebase' mode later */
+static inline int is_rebase_i(const struct replay_opts *opts)
+{
+   return 0;
+}
+
 static const char *get_dir(const struct replay_opts *opts)
 {
return git_path_seq_dir();
@@ -370,19 +383,79 @@ static int is_index_unchanged(void)
 }
 
 /*
+ * Read the author-script file into an environment block, ready for use in
+ * run_command(), that can be free()d afterwards.
+ */
+static char **read_author_script(void)
+{
+   struct strbuf script = STRBUF_INIT;
+   int i, count = 0;
+   char *p, *p2, **env;
+   size_t env_size;
+
+   if (strbuf_read_file(, rebase_path_author_script(), 256) <= 0)
+   return NULL;
+
+   for (p = script.buf; *p; p++)
+   if (skip_prefix(p, "'''", (const char **)))
+   strbuf_splice(, p - script.buf, p2 - p, "'", 1);
+   else if (*p == '\'')
+   strbuf_splice(, p-- - script.buf, 1, "", 0);
+   else if (*p == '\n') {
+   *p = '\0';
+   count++;
+   }
+
+   env_size = (count + 1) * sizeof(*env);
+   strbuf_grow(, env_size);
+   memmove(script.buf + env_size, script.buf, script.len);
+   p = script.buf + env_size;
+   env = (char **)strbuf_detach(, NULL);
+
+   for (i = 0; i < count; i++) {
+   env[i] = p;
+   p += strlen(p) + 1;
+   }
+   env[count] = NULL;
+
+   return env;
+}
+
+/*
  * If we are cherry-pick, and if the merge did not result in
  * hand-editing, we will hit this commit and inherit the original
  * author date and name.
+ *
  * If we are revert, or if our cherry-pick results in a hand merge,
  * we had better say that the current user is responsible for that.
+ *
+ * An exception is when run_git_commit() is called during an
+ * interactive rebase: in that case, we will want to retain the
+ * author metadata.
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts,
  int allow_empty)
 {
+   char **env = NULL;
struct argv_array array;
int rc;
const char *value;
 
+   if (is_rebase_i(opts)) {
+   env = read_author_script();
+   if (!env)
+   return error("You have staged changes in your working "
+   "tree. If these changes are meant to be\n"
+   "squashed into the previous commit, run:\n\n"
+   "  git commit --amend $gpg_sign_opt_quoted\n\n"
+   "If they are meant to go into a new commit, "
+   "run:\n\n"
+   "  git commit $gpg_sign_opt_quoted\n\n"
+   "In both cases, once you're done, continue "
+   "with:\n\n"
+   "  git rebase --continue\n");
+   }
+
argv_array_init();
argv_array_push(, "commit");
argv_array_push(, "-n");
@@ -391,14 +464,13 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_pushf(, "-S%s", opts->gpg_sign);
if (opts->signoff)
argv_array_push(, "-s");
-   if (!opts->edit) {
-   argv_array_push(, "-F");
-   argv_array_push(, defmsg);
-   if (!opts->signoff &&
-

[PATCH v5 15/27] sequencer: allow editing the commit message on a case-by-case basis

[Johannes Schindelin]

In the upcoming commits, we will implement more and more of rebase -i's
functionality inside the sequencer. One particular feature of the
commands to come is that some of them allow editing the commit message
while others don't, i.e. we cannot define in the replay_opts whether the
commit message should be edited or not.

Let's add a new parameter to the run_git_commit() function. Previously,
it was the duty of the caller to ensure that the opts->edit setting
indicates whether to let the user edit the commit message or not,
indicating that it is an "all or nothing" setting, i.e. that the
sequencer wants to let the user edit *all* commit message, or none at
all. In the upcoming rebase -i mode, it will depend on the particular
command that is currently executed, though.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 48 
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 282c4d1..c0a0aa0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -15,6 +15,7 @@
 #include "merge-recursive.h"
 #include "refs.h"
 #include "argv-array.h"
+#include "quote.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -33,6 +34,11 @@ static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
  * being rebased.
  */
 static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
+/*
+ * The following files are written by git-rebase just after parsing the
+ * command-line (and are only consumed, not modified, by the sequencer).
+ */
+static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
 
 /* We will introduce the 'interactive rebase' mode later */
 static inline int is_rebase_i(const struct replay_opts *opts)
@@ -132,6 +138,16 @@ static int has_conforming_footer(struct strbuf *sb, struct 
strbuf *sob,
return 1;
 }
 
+static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
+{
+   static struct strbuf buf = STRBUF_INIT;
+
+   strbuf_reset();
+   if (opts->gpg_sign)
+   sq_quotef(, "-S%s", opts->gpg_sign);
+   return buf.buf;
+}
+
 int sequencer_remove_state(struct replay_opts *opts)
 {
struct strbuf dir = STRBUF_INIT;
@@ -468,7 +484,7 @@ static char **read_author_script(void)
  * author metadata.
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts,
- int allow_empty)
+ int allow_empty, int edit)
 {
char **env = NULL;
struct argv_array array;
@@ -477,17 +493,20 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
 
if (is_rebase_i(opts)) {
env = read_author_script();
-   if (!env)
+   if (!env) {
+   const char *gpg_opt = gpg_sign_opt_quoted(opts);
+
return error("You have staged changes in your working "
"tree. If these changes are meant to be\n"
"squashed into the previous commit, run:\n\n"
-   "  git commit --amend $gpg_sign_opt_quoted\n\n"
+   "  git commit --amend %s\n\n"
"If they are meant to go into a new commit, "
"run:\n\n"
-   "  git commit $gpg_sign_opt_quoted\n\n"
+   "  git commit %s\n\n"
"In both cases, once you're done, continue "
"with:\n\n"
-   "  git rebase --continue\n");
+   "  git rebase --continue\n", gpg_opt, gpg_opt);
+   }
}
 
argv_array_init();
@@ -500,7 +519,7 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_push(, "-s");
if (defmsg)
argv_array_pushl(, "-F", defmsg, NULL);
-   if (opts->edit)
+   if (edit)
argv_array_push(, "-e");
else if (!opts->signoff && !opts->record_origin &&
 git_config_get_value("commit.cleanup", ))
@@ -767,7 +786,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
}
if (!opts->no_commit)
res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
-opts, allow);
+opts, allow, opts->edit);
 
 leave:
free_message(commit, );
@@ -989,8 +1008,21 @@ static int populate_opts_cb(const char *key, const char 
*value, void *data)
 
 static int read_populate_opts(struct replay_opts *opts)
 {
-   if (is_rebase_i(opts))
+   if (is_rebase_i(opts)) {
+   struct strbuf buf = STRBUF_INIT;
+
+   if (read_oneliner(, rebase_path_gpg_sign_opt(), 1)) {
+   if (!starts_with(buf.buf, "-S"))
+ 

[PATCH v5 10/27] sequencer: avoid completely different messages for different actions

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 04fcfd8..120a8ee 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -229,11 +229,8 @@ static int error_dirty_index(struct replay_opts *opts)
if (read_cache_unmerged())
return error_resolve_conflict(action_name(opts));
 
-   /* Different translation strings for cherry-pick and revert */
-   if (opts->action == REPLAY_PICK)
-   error(_("Your local changes would be overwritten by 
cherry-pick."));
-   else
-   error(_("Your local changes would be overwritten by revert."));
+   error(_("Your local changes would be overwritten by %s."),
+   action_name(opts));
 
if (advice_commit_before_merge)
advise(_("Commit your changes or stash them to proceed."));
-- 
2.10.1.583.g721a9e0

[PATCH v5 09/27] sequencer: strip CR from the todo script

[Johannes Schindelin]

It is not unheard of that editors on Windows write CR/LF even if the
file originally had only LF. This is particularly awkward for exec lines
of a rebase -i todo sheet. Take for example the insn "exec echo": The
shell script parser splits at the LF and leaves the CR attached to
"echo", which leads to the unknown command "echo\r".

Work around that by stripping CR when reading the todo commands, as we
already do for LF.

This happens to fix t9903.14 and .15 in MSYS1 environments (with the
rebase--helper patches based on this patch series): the todo script
constructed in such a setup contains CR/LF thanks to MSYS1 runtime's
cleverness.

Based on a report and a patch by Johannes Sixt.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 145de78..04fcfd8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -776,6 +776,9 @@ static int parse_insn_buffer(char *buf, struct todo_list 
*todo_list)
 
next_p = *eol ? eol + 1 /* skip LF */ : eol;
 
+   if (p != eol && eol[-1] == '\r')
+   eol--; /* strip Carriage Return */
+
item = append_new_todo(todo_list);
item->offset_in_buf = p - todo_list->buf.buf;
if (parse_insn_line(item, p, eol)) {
-- 
2.10.1.583.g721a9e0

[PATCH v5 11/27] sequencer: get rid of the subcommand field

[Johannes Schindelin]

The subcommands are used exactly once, at the very beginning of
sequencer_pick_revisions(), to determine what to do. This is an
unnecessary level of indirection: we can simply call the correct
function to begin with. So let's do that.

While at it, ensure that the subcommands return an error code so that
they do not have to die() all over the place (bad practice for library
functions...).

Signed-off-by: Johannes Schindelin 
---
 builtin/revert.c | 36 
 sequencer.c  | 35 +++
 sequencer.h  | 13 -
 3 files changed, 31 insertions(+), 53 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index ba5a88c..4ca5b51 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -71,7 +71,7 @@ static void verify_opt_compatible(const char *me, const char 
*base_opt, ...)
die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt);
 }
 
-static void parse_args(int argc, const char **argv, struct replay_opts *opts)
+static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 {
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
const char *me = action_name(opts);
@@ -115,25 +115,15 @@ static void parse_args(int argc, const char **argv, 
struct replay_opts *opts)
if (opts->keep_redundant_commits)
opts->allow_empty = 1;
 
-   /* Set the subcommand */
-   if (cmd == 'q')
-   opts->subcommand = REPLAY_REMOVE_STATE;
-   else if (cmd == 'c')
-   opts->subcommand = REPLAY_CONTINUE;
-   else if (cmd == 'a')
-   opts->subcommand = REPLAY_ROLLBACK;
-   else
-   opts->subcommand = REPLAY_NONE;
-
/* Check for incompatible command line arguments */
-   if (opts->subcommand != REPLAY_NONE) {
+   if (cmd) {
char *this_operation;
-   if (opts->subcommand == REPLAY_REMOVE_STATE)
+   if (cmd == 'q')
this_operation = "--quit";
-   else if (opts->subcommand == REPLAY_CONTINUE)
+   else if (cmd == 'c')
this_operation = "--continue";
else {
-   assert(opts->subcommand == REPLAY_ROLLBACK);
+   assert(cmd == 'a');
this_operation = "--abort";
}
 
@@ -156,7 +146,7 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
"--edit", opts->edit,
NULL);
 
-   if (opts->subcommand != REPLAY_NONE) {
+   if (cmd) {
opts->revs = NULL;
} else {
struct setup_revision_opt s_r_opt;
@@ -178,6 +168,14 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
/* These option values will be free()d */
opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
opts->strategy = xstrdup_or_null(opts->strategy);
+
+   if (cmd == 'q')
+   return sequencer_remove_state(opts);
+   if (cmd == 'c')
+   return sequencer_continue(opts);
+   if (cmd == 'a')
+   return sequencer_rollback(opts);
+   return sequencer_pick_revisions(opts);
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
@@ -189,8 +187,7 @@ int cmd_revert(int argc, const char **argv, const char 
*prefix)
opts.edit = 1;
opts.action = REPLAY_REVERT;
git_config(git_default_config, NULL);
-   parse_args(argc, argv, );
-   res = sequencer_pick_revisions();
+   res = run_sequencer(argc, argv, );
if (res < 0)
die(_("revert failed"));
return res;
@@ -203,8 +200,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char 
*prefix)
 
opts.action = REPLAY_PICK;
git_config(git_default_config, NULL);
-   parse_args(argc, argv, );
-   res = sequencer_pick_revisions();
+   res = run_sequencer(argc, argv, );
if (res < 0)
die(_("cherry-pick failed"));
return res;
diff --git a/sequencer.c b/sequencer.c
index 120a8ee..9f22c5e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -119,7 +119,7 @@ static int has_conforming_footer(struct strbuf *sb, struct 
strbuf *sob,
return 1;
 }
 
-static void remove_sequencer_state(const struct replay_opts *opts)
+int sequencer_remove_state(struct replay_opts *opts)
 {
struct strbuf dir = STRBUF_INIT;
int i;
@@ -133,6 +133,8 @@ static void remove_sequencer_state(const struct replay_opts 
*opts)
strbuf_addf(, "%s", get_dir(opts));
remove_dir_recursively(, 0);
strbuf_release();
+
+   return 0;
 }
 
 static const char *action_name(const struct replay_opts *opts)
@@ -975,7 +977,7 @@ static int rollback_single_pick(void)
return reset_for_rollback(head_sha1);
 }
 

[PATCH v5 07/27] sequencer: refactor the code to obtain a short commit name

[Johannes Schindelin]

Not only does this DRY up the code (providing a better documentation what
the code is about, as well as allowing to change the behavior in a single
place), it also makes it substantially shorter to use the same
functionality in functions to be introduced when we teach the sequencer to
process interactive-rebase's git-rebase-todo file.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index fb0b94b..499f5ee 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -147,13 +147,18 @@ struct commit_message {
const char *message;
 };
 
+static const char *short_commit_name(struct commit *commit)
+{
+   return find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV);
+}
+
 static int get_message(struct commit *commit, struct commit_message *out)
 {
const char *abbrev, *subject;
int subject_len;
 
out->message = logmsg_reencode(commit, NULL, 
get_commit_output_encoding());
-   abbrev = find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV);
+   abbrev = short_commit_name(commit);
 
subject_len = find_commit_subject(out->message, );
 
@@ -621,8 +626,7 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
error(opts->action == REPLAY_REVERT
  ? _("could not revert %s... %s")
  : _("could not apply %s... %s"),
- find_unique_abbrev(commit->object.oid.hash, 
DEFAULT_ABBREV),
- msg.subject);
+ short_commit_name(commit), msg.subject);
print_advice(res == 1, opts);
rerere(opts->allow_rerere_auto);
goto leave;
-- 
2.10.1.583.g721a9e0

[PATCH v5 12/27] sequencer: remember the onelines when parsing the todo file

[Johannes Schindelin]

The `git-rebase-todo` file contains a list of commands. Most of those
commands have the form

  

The  is displayed primarily for the user's convenience, as
rebase -i really interprets only the   part. However, there
are *some* places in interactive rebase where the  is used to
display messages, e.g. for reporting at which commit we stopped.

So let's just remember it when parsing the todo file; we keep a copy of
the entire todo file anyway (to write out the new `done` and
`git-rebase-todo` file just before processing each command), so all we
need to do is remember the begin offsets and lengths.

As we will have to parse and remember the command-line for `exec` commands
later, we do not call the field "oneline" but rather "arg" (and will reuse
that for exec's command-line).

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 9f22c5e..3d1fdac 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -706,6 +706,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
 struct todo_item {
enum todo_command command;
struct commit *commit;
+   const char *arg;
+   int arg_len;
size_t offset_in_buf;
 };
 
@@ -757,6 +759,9 @@ static int parse_insn_line(struct todo_item *item, const 
char *bol, char *eol)
status = get_sha1(bol, commit_sha1);
*end_of_object_name = saved;
 
+   item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
+   item->arg_len = (int)(eol - item->arg);
+
if (status < 0)
return -1;
 
@@ -907,6 +912,8 @@ static int walk_revs_populate_todo(struct todo_list 
*todo_list,
 
item->command = command;
item->commit = commit;
+   item->arg = NULL;
+   item->arg_len = 0;
item->offset_in_buf = todo_list->buf.len;
subject_len = find_commit_subject(commit_buffer, );
strbuf_addf(_list->buf, "%s %s %.*s\n", command_string,
-- 
2.10.1.583.g721a9e0

[PATCH v5 05/27] sequencer: plug memory leaks for the option values

[Johannes Schindelin]

The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
like a one-shot command when it reads its configuration: memory is
allocated and released only when the command exits.

This is kind of okay for git-cherry-pick, which *is* a one-shot
command. All the work to make the sequencer its work horse was
done to allow using the functionality as a library function, though,
including proper clean-up after use.

To remedy that, we now take custody of the option values in question,
requiring those values to be malloc()ed or strdup()ed (an alternative
approach, to add a list of pointers to malloc()ed data and to ask the
sequencer to release all of them in the end, was proposed earlier but
rejected during review).

Note: this means that we now have to take care to strdup() the values
passed via the command-line.

Signed-off-by: Johannes Schindelin 
---
 builtin/revert.c |  4 
 sequencer.c  | 26 ++
 sequencer.h  |  6 +++---
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 7365559..ba5a88c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -174,6 +174,10 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
 
if (argc > 1)
usage_with_options(usage_str, options);
+
+   /* These option values will be free()d */
+   opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
+   opts->strategy = xstrdup_or_null(opts->strategy);
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/sequencer.c b/sequencer.c
index 8d272fb..04c55f2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -117,6 +117,13 @@ static int has_conforming_footer(struct strbuf *sb, struct 
strbuf *sob,
 static void remove_sequencer_state(const struct replay_opts *opts)
 {
struct strbuf dir = STRBUF_INIT;
+   int i;
+
+   free(opts->gpg_sign);
+   free(opts->strategy);
+   for (i = 0; i < opts->xopts_nr; i++)
+   free(opts->xopts[i]);
+   free(opts->xopts);
 
strbuf_addf(, "%s", get_dir(opts));
remove_dir_recursively(, 0);
@@ -280,7 +287,7 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
struct merge_options o;
struct tree *result, *next_tree, *base_tree, *head_tree;
int clean;
-   const char **xopt;
+   char **xopt;
static struct lock_file index_lock;
 
hold_locked_index(_lock, 1);
@@ -583,7 +590,8 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
 
commit_list_insert(base, );
commit_list_insert(next, );
-   res |= try_merge_command(opts->strategy, opts->xopts_nr, 
opts->xopts,
+   res |= try_merge_command(opts->strategy,
+opts->xopts_nr, (const char 
**)opts->xopts,
common, sha1_to_hex(head), remotes);
free_commit_list(common);
free_commit_list(remotes);
@@ -783,6 +791,16 @@ static int read_populate_todo(struct commit_list 
**todo_list,
return 0;
 }
 
+static int git_config_string_dup(char **dest,
+const char *var, const char *value)
+{
+   if (!value)
+   return config_error_nonbool(var);
+   free(*dest);
+   *dest = xstrdup(value);
+   return 0;
+}
+
 static int populate_opts_cb(const char *key, const char *value, void *data)
 {
struct replay_opts *opts = data;
@@ -803,9 +821,9 @@ static int populate_opts_cb(const char *key, const char 
*value, void *data)
else if (!strcmp(key, "options.mainline"))
opts->mainline = git_config_int(key, value);
else if (!strcmp(key, "options.strategy"))
-   git_config_string(>strategy, key, value);
+   git_config_string_dup(>strategy, key, value);
else if (!strcmp(key, "options.gpg-sign"))
-   git_config_string(>gpg_sign, key, value);
+   git_config_string_dup(>gpg_sign, key, value);
else if (!strcmp(key, "options.strategy-option")) {
ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
opts->xopts[opts->xopts_nr++] = xstrdup(value);
diff --git a/sequencer.h b/sequencer.h
index dd4d33a..8453669 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -34,11 +34,11 @@ struct replay_opts {
 
int mainline;
 
-   const char *gpg_sign;
+   char *gpg_sign;
 
/* Merge strategy */
-   const char *strategy;
-   const char **xopts;
+   char *strategy;
+   char **xopts;
size_t xopts_nr, xopts_alloc;
 
/* Only used by REPLAY_NONE */
-- 
2.10.1.583.g721a9e0

[PATCH v5 17/27] sequencer: support cleaning up commit messages

[Johannes Schindelin]

The run_git_commit() function already knows how to amend commits, and
with this new option, it can also clean up commit messages (i.e. strip
out commented lines). This is needed to implement rebase -i's 'fixup'
and 'squash' commands as sequencer commands.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 1ef50a0..8646ca5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -484,7 +484,8 @@ static char **read_author_script(void)
  * author metadata.
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts,
- int allow_empty, int edit, int amend)
+ int allow_empty, int edit, int amend,
+ int cleanup_commit_message)
 {
char **env = NULL;
struct argv_array array;
@@ -521,9 +522,12 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_push(, "-s");
if (defmsg)
argv_array_pushl(, "-F", defmsg, NULL);
+   if (cleanup_commit_message)
+   argv_array_push(, "--cleanup=strip");
if (edit)
argv_array_push(, "-e");
-   else if (!opts->signoff && !opts->record_origin &&
+   else if (!cleanup_commit_message &&
+!opts->signoff && !opts->record_origin &&
 git_config_get_value("commit.cleanup", ))
argv_array_push(, "--cleanup=verbatim");
 
@@ -788,7 +792,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
}
if (!opts->no_commit)
res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
-opts, allow, opts->edit, 0);
+opts, allow, opts->edit, 0, 0);
 
 leave:
free_message(commit, );
-- 
2.10.1.583.g721a9e0

[PATCH v5 06/27] sequencer: future-proof read_populate_todo()

[Johannes Schindelin]

Over the next commits, we will work on improving the sequencer to the
point where it can process the todo script of an interactive rebase. To
that end, we will need to teach the sequencer to read interactive
rebase's todo file. In preparation, we consolidate all places where
that todo file is needed to call a function that we will later extend.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 04c55f2..fb0b94b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -32,6 +32,11 @@ static const char *get_dir(const struct replay_opts *opts)
return git_path_seq_dir();
 }
 
+static const char *get_todo_path(const struct replay_opts *opts)
+{
+   return git_path_todo_file();
+}
+
 static int is_rfc2822_line(const char *buf, int len)
 {
int i;
@@ -769,25 +774,24 @@ static int parse_insn_buffer(char *buf, struct 
commit_list **todo_list,
 static int read_populate_todo(struct commit_list **todo_list,
struct replay_opts *opts)
 {
+   const char *todo_file = get_todo_path(opts);
struct strbuf buf = STRBUF_INIT;
int fd, res;
 
-   fd = open(git_path_todo_file(), O_RDONLY);
+   fd = open(todo_file, O_RDONLY);
if (fd < 0)
-   return error_errno(_("Could not open %s"),
-  git_path_todo_file());
+   return error_errno(_("Could not open %s"), todo_file);
if (strbuf_read(, fd, 0) < 0) {
close(fd);
strbuf_release();
-   return error(_("Could not read %s."), git_path_todo_file());
+   return error(_("Could not read %s."), todo_file);
}
close(fd);
 
res = parse_insn_buffer(buf.buf, todo_list, opts);
strbuf_release();
if (res)
-   return error(_("Unusable instruction sheet: %s"),
-   git_path_todo_file());
+   return error(_("Unusable instruction sheet: %s"), todo_file);
return 0;
 }
 
@@ -1075,7 +1079,7 @@ static int sequencer_continue(struct replay_opts *opts)
 {
struct commit_list *todo_list = NULL;
 
-   if (!file_exists(git_path_todo_file()))
+   if (!file_exists(get_todo_path(opts)))
return continue_single_pick();
if (read_populate_opts(opts) ||
read_populate_todo(_list, opts))
-- 
2.10.1.583.g721a9e0

[PATCH v5 08/27] sequencer: completely revamp the "todo" script parsing

[Johannes Schindelin]

When we came up with the "sequencer" idea, we really wanted to have
kind of a plumbing equivalent of the interactive rebase. Hence the
choice of words: the "todo" script, a "pick", etc.

However, when it came time to implement the entire shebang, somehow this
idea got lost and the sequencer was used as working horse for
cherry-pick and revert instead. So as not to interfere with the
interactive rebase, it even uses a separate directory to store its
state.

Furthermore, it also is stupidly strict about the "todo" script it
accepts: while it parses commands in a way that was *designed* to be
similar to the interactive rebase, it then goes on to *error out* if the
commands disagree with the overall action (cherry-pick or revert).

Finally, the sequencer code chose to deviate from the interactive rebase
code insofar that when it comes to writing the file with the remaining
commands, it *reformats* the "todo" script instead of just writing the
part of the parsed script that were not yet processed. This is not only
unnecessary churn, but might well lose information that is valuable to
the user (i.e. comments after the commands).

Let's just bite the bullet and rewrite the entire parser; the code now
becomes not only more elegant: it allows us to go on and teach the
sequencer how to parse *true* "todo" scripts as used by the interactive
rebase itself. In a way, the sequencer is about to grow up to do its
older brother's job. Better.

In particular, we choose to maintain the list of commands in an array
instead of a linked list: this is flexible enough to allow us later on to
even implement rebase -i's reordering of fixup!/squash! commits very
easily (and with a very nice speed bonus, at least on Windows).

While at it, do not stop at the first problem, but list *all* of the
problems. This will help the user when the sequencer will do `rebase
-i`'s work by allowing to address all issues in one go rather than going
back and forth until the todo list is valid.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 284 ++--
 1 file changed, 163 insertions(+), 121 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 499f5ee..145de78 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -470,7 +470,26 @@ static int allow_empty(struct replay_opts *opts, struct 
commit *commit)
return 1;
 }
 
-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
+enum todo_command {
+   TODO_PICK = 0,
+   TODO_REVERT
+};
+
+static const char *todo_command_strings[] = {
+   "pick",
+   "revert"
+};
+
+static const char *command_to_string(const enum todo_command command)
+{
+   if (command < ARRAY_SIZE(todo_command_strings))
+   return todo_command_strings[command];
+   die("Unknown command: %d", command);
+}
+
+
+static int do_pick_commit(enum todo_command command, struct commit *commit,
+   struct replay_opts *opts)
 {
unsigned char head[20];
struct commit *base, *next, *parent;
@@ -529,10 +548,11 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
return fast_forward_to(commit->object.oid.hash, head, unborn, 
opts);
 
if (parent && parse_commit(parent) < 0)
-   /* TRANSLATORS: The first %s will be "revert" or
-  "cherry-pick", the second %s a SHA1 */
+   /* TRANSLATORS: The first %s will be a "todo" command like
+  "revert" or "pick", the second %s a SHA1. */
return error(_("%s: cannot parse parent commit %s"),
-   action_name(opts), oid_to_hex(>object.oid));
+   command_to_string(command),
+   oid_to_hex(>object.oid));
 
if (get_message(commit, ) != 0)
return error(_("Cannot get commit message for %s"),
@@ -545,7 +565,7 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
 * reverse of it if we are revert.
 */
 
-   if (opts->action == REPLAY_REVERT) {
+   if (command == TODO_REVERT) {
base = commit;
base_label = msg.label;
next = parent;
@@ -586,7 +606,7 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
}
}
 
-   if (!opts->strategy || !strcmp(opts->strategy, "recursive") || 
opts->action == REPLAY_REVERT) {
+   if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command 
== TODO_REVERT) {
res = do_recursive_merge(base, next, base_label, next_label,
 head, , opts);
if (res < 0)
@@ -613,17 +633,17 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
 * However, if the merge did not even start, then we don't want to
 * write it at all.
 */
-   if (opts->action 

[PATCH v5 18/27] sequencer: left-trim lines read from the script

[Johannes Schindelin]

Interactive rebase's scripts may be indented; we need to handle this
case, too, now that we prepare the sequencer to process interactive
rebases.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 8646ca5..d74fdce 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -874,6 +874,9 @@ static int parse_insn_line(struct todo_item *item, const 
char *bol, char *eol)
char *end_of_object_name;
int i, saved, status, padding;
 
+   /* left-trim */
+   bol += strspn(bol, " \t");
+
for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++)
if (skip_prefix(bol, todo_command_strings[i], )) {
item->command = i;
-- 
2.10.1.583.g721a9e0

[PATCH v5 16/27] sequencer: support amending commits

[Johannes Schindelin]

This teaches the run_git_commit() function to take an argument that will
allow us to implement "todo" commands that need to amend the commit
messages ("fixup", "squash" and "reword").

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c0a0aa0..1ef50a0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -484,7 +484,7 @@ static char **read_author_script(void)
  * author metadata.
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts,
- int allow_empty, int edit)
+ int allow_empty, int edit, int amend)
 {
char **env = NULL;
struct argv_array array;
@@ -513,6 +513,8 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_push(, "commit");
argv_array_push(, "-n");
 
+   if (amend)
+   argv_array_push(, "--amend");
if (opts->gpg_sign)
argv_array_pushf(, "-S%s", opts->gpg_sign);
if (opts->signoff)
@@ -786,7 +788,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
}
if (!opts->no_commit)
res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
-opts, allow, opts->edit);
+opts, allow, opts->edit, 0);
 
 leave:
free_message(commit, );
-- 
2.10.1.583.g721a9e0

[PATCH v5 14/27] sequencer: introduce a helper to read files written by scripts

[Johannes Schindelin]

As we are slowly teaching the sequencer to perform the hard work for
the interactive rebase, we need to read files that were written by
shell scripts.

These files typically contain a single line and are invariably ended
by a line feed (and possibly a carriage return before that). Let's use
a helper to read such files and to remove the line ending.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 6d5fe94..282c4d1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -234,6 +234,40 @@ static int write_message(struct strbuf *msgbuf, const char 
*filename)
return 0;
 }
 
+/*
+ * Reads a file that was presumably written by a shell script, i.e. with an
+ * end-of-line marker that needs to be stripped.
+ *
+ * Note that only the last end-of-line marker is stripped, consistent with the
+ * behavior of "$(cat path)" in a shell script.
+ *
+ * Returns 1 if the file was read, 0 if it could not be read or does not exist.
+ */
+static int read_oneliner(struct strbuf *buf,
+   const char *path, int skip_if_empty)
+{
+   int orig_len = buf->len;
+
+   if (!file_exists(path))
+   return 0;
+
+   if (strbuf_read_file(buf, path, 0) < 0) {
+   warning_errno(_("could not read '%s'"), path);
+   return 0;
+   }
+
+   if (buf->len > orig_len && buf->buf[buf->len - 1] == '\n') {
+   if (--buf->len > orig_len && buf->buf[buf->len - 1] == '\r')
+   --buf->len;
+   buf->buf[buf->len] = '\0';
+   }
+
+   if (skip_if_empty && buf->len == orig_len)
+   return 0;
+
+   return 1;
+}
+
 static struct tree *empty_tree(void)
 {
return lookup_tree(EMPTY_TREE_SHA1_BIN);
-- 
2.10.1.583.g721a9e0

[PATCH v5 00/27] Prepare the sequencer for the upcoming rebase -i patches

[Johannes Schindelin]

This patch series marks the '4' in the countdown to speed up rebase -i
by implementing large parts in C (read: there will be three more patch
series after that before the full benefit hits git.git: sequencer-i,
rebase--helper and rebase-i-extra). It is based on the `libify-sequencer`
patch series I submitted earlier.

The patches in this series merely prepare the sequencer code for the
next patch series that actually teaches the sequencer to run rebase -i's
commands.

The reason to split these two patch series is simple: to keep them at a
sensible size.

The two patch series after that are much smaller: a two-patch "series"
that switches rebase -i to use the sequencer (except with --root or
--preserve-merges), and a couple of patches to move several pretty
expensive script processing steps to C (think: autosquash).

The end game of this patch series is a git-rebase--helper that makes
rebase -i 5x faster on Windows (according to t/perf/p3404). Travis says
that even MacOSX and Linux benefit (4x and 3x, respectively).

I have been working on this since early February, whenever time allowed,
and it is time to put it into the users' hands. To that end, I already
integrated the whole shebang into Git for Windows 2.10.0 and 2.10.1
where it has been running without complaints (and some quite positive
feedback).

It would be *really* nice if we could get this patch series at least into
`next` soon, as it gets late and later for the rest of the patches to make
it into `master` in time for v2.11 (and it is not for lack of trying on my
end...).

Changes vs v4:

- clarified in a code comment that read_oneliner() is lenient when it
  comes to multi-line files: it still reads the entire file, but strips
  off only the final EOL (if any).

- rephrased a commit message to stop judging Junio's taste ;-)

- changed "strip LF" to "skip LF", as requested.

- rephrased a commit message to talk about pluggin memory leaks instead
  of stating that the memory is now eventually released.

- dropped the "sequencer: do not try to commit when there were merge
  conflicts" patch that appears to be no longer necessary.

- split and modified the commit refactoring write_message() according to
  Junio's suggestions.


Johannes Schindelin (27):
  sequencer: use static initializers for replay_opts
  sequencer: use memoized sequencer directory path
  sequencer: avoid unnecessary indirection
  sequencer: future-proof remove_sequencer_state()
  sequencer: plug memory leaks for the option values
  sequencer: future-proof read_populate_todo()
  sequencer: refactor the code to obtain a short commit name
  sequencer: completely revamp the "todo" script parsing
  sequencer: strip CR from the todo script
  sequencer: avoid completely different messages for different actions
  sequencer: get rid of the subcommand field
  sequencer: remember the onelines when parsing the todo file
  sequencer: prepare for rebase -i's commit functionality
  sequencer: introduce a helper to read files written by scripts
  sequencer: allow editing the commit message on a case-by-case basis
  sequencer: support amending commits
  sequencer: support cleaning up commit messages
  sequencer: left-trim lines read from the script
  sequencer: stop releasing the strbuf in write_message()
  sequencer: roll back lock file if write_message() failed
  sequencer: refactor write_message() to take a pointer/length
  sequencer: teach write_message() to append an optional LF
  sequencer: remove overzealous assumption in rebase -i mode
  sequencer: mark action_name() for translation
  sequencer: quote filenames in error messages
  sequencer: start error messages consistently with lower case
  sequencer: mark all error messages for translation

 builtin/commit.c  |   2 +-
 builtin/revert.c  |  46 ++-
 sequencer.c   | 680 --
 sequencer.h   |  23 +-
 t/t3501-revert-cherry-pick.sh |   2 +-
 5 files changed, 492 insertions(+), 261 deletions(-)


base-commit: 659889482ac63411daea38b2c3d127842ea04e4d
Published-As: https://github.com/dscho/git/releases/tag/prepare-sequencer-v5
Fetch-It-Via: git fetch https://github.com/dscho/git prepare-sequencer-v5

Interdiff vs v4:

 diff --git a/sequencer.c b/sequencer.c
 index 1cf70f7..a61fe76 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -234,8 +234,8 @@ static void print_advice(int show_hint, struct replay_opts 
*opts)
}
  }
  
 -static int write_with_lock_file(const char *filename,
 -  const void *buf, size_t len, int append_eol)
 +static int write_message(const void *buf, size_t len, const char *filename,
 +   int append_eol)
  {
static struct lock_file msg_file;
  
 @@ -258,16 +258,12 @@ static int write_with_lock_file(const char *filename,
return 0;
  }
  
 -static int write_message(struct strbuf *msgbuf, const char *filename)
 -{
 -  int res = write_with_lock_file(filename, 

[PATCH v5 01/27] sequencer: use static initializers for replay_opts

[Johannes Schindelin]

This change is not completely faithful: instead of initializing all fields
to 0, we choose to initialize command and subcommand to -1 (instead of
defaulting to REPLAY_REVERT and REPLAY_NONE, respectively). Practically,
it makes no difference at all, but future-proofs the code to require
explicit assignments for both fields.

Signed-off-by: Johannes Schindelin 
---
 builtin/revert.c | 6 ++
 sequencer.h  | 1 +
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 4e69380..7365559 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -178,10 +178,9 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
-   struct replay_opts opts;
+   struct replay_opts opts = REPLAY_OPTS_INIT;
int res;
 
-   memset(, 0, sizeof(opts));
if (isatty(0))
opts.edit = 1;
opts.action = REPLAY_REVERT;
@@ -195,10 +194,9 @@ int cmd_revert(int argc, const char **argv, const char 
*prefix)
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
-   struct replay_opts opts;
+   struct replay_opts opts = REPLAY_OPTS_INIT;
int res;
 
-   memset(, 0, sizeof(opts));
opts.action = REPLAY_PICK;
git_config(git_default_config, NULL);
parse_args(argc, argv, );
diff --git a/sequencer.h b/sequencer.h
index 5ed5cb1..db425ad 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -47,6 +47,7 @@ struct replay_opts {
/* Only used by REPLAY_NONE */
struct rev_info *revs;
 };
+#define REPLAY_OPTS_INIT { -1, -1 }
 
 int sequencer_pick_revisions(struct replay_opts *opts);
 
-- 
2.10.1.583.g721a9e0

[PATCH v5 02/27] sequencer: use memoized sequencer directory path

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin 
---
 builtin/commit.c |  2 +-
 sequencer.c  | 11 ++-
 sequencer.h  |  5 +
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1cba3b7..9fddb19 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -183,7 +183,7 @@ static void determine_whence(struct wt_status *s)
whence = FROM_MERGE;
else if (file_exists(git_path_cherry_pick_head())) {
whence = FROM_CHERRY_PICK;
-   if (file_exists(git_path(SEQ_DIR)))
+   if (file_exists(git_path_seq_dir()))
sequencer_in_use = 1;
}
else
diff --git a/sequencer.c b/sequencer.c
index eec8a60..cb16cbd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -21,10 +21,11 @@
 const char sign_off_header[] = "Signed-off-by: ";
 static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
-static GIT_PATH_FUNC(git_path_todo_file, SEQ_TODO_FILE)
-static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)
-static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)
-static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)
+GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
+
+static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
+static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
+static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
 
 static int is_rfc2822_line(const char *buf, int len)
 {
@@ -112,7 +113,7 @@ static void remove_sequencer_state(void)
 {
struct strbuf seq_dir = STRBUF_INIT;
 
-   strbuf_addstr(_dir, git_path(SEQ_DIR));
+   strbuf_addstr(_dir, git_path_seq_dir());
remove_dir_recursively(_dir, 0);
strbuf_release(_dir);
 }
diff --git a/sequencer.h b/sequencer.h
index db425ad..dd4d33a 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -1,10 +1,7 @@
 #ifndef SEQUENCER_H
 #define SEQUENCER_H
 
-#define SEQ_DIR"sequencer"
-#define SEQ_HEAD_FILE  "sequencer/head"
-#define SEQ_TODO_FILE  "sequencer/todo"
-#define SEQ_OPTS_FILE  "sequencer/opts"
+const char *git_path_seq_dir(void);
 
 #define APPEND_SIGNOFF_DEDUP (1u << 0)
 
-- 
2.10.1.583.g721a9e0

[PATCH v5 04/27] sequencer: future-proof remove_sequencer_state()

[Johannes Schindelin]

In a couple of commits, we will teach the sequencer to handle the
nitty gritty of the interactive rebase, which keeps its state in a
different directory.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c2fbf6f..8d272fb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,6 +27,11 @@ static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
 static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
 static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
 
+static const char *get_dir(const struct replay_opts *opts)
+{
+   return git_path_seq_dir();
+}
+
 static int is_rfc2822_line(const char *buf, int len)
 {
int i;
@@ -109,13 +114,13 @@ static int has_conforming_footer(struct strbuf *sb, 
struct strbuf *sob,
return 1;
 }
 
-static void remove_sequencer_state(void)
+static void remove_sequencer_state(const struct replay_opts *opts)
 {
-   struct strbuf seq_dir = STRBUF_INIT;
+   struct strbuf dir = STRBUF_INIT;
 
-   strbuf_addstr(_dir, git_path_seq_dir());
-   remove_dir_recursively(_dir, 0);
-   strbuf_release(_dir);
+   strbuf_addf(, "%s", get_dir(opts));
+   remove_dir_recursively(, 0);
+   strbuf_release();
 }
 
 static const char *action_name(const struct replay_opts *opts)
@@ -940,7 +945,7 @@ static int sequencer_rollback(struct replay_opts *opts)
}
if (reset_for_rollback(sha1))
goto fail;
-   remove_sequencer_state();
+   remove_sequencer_state(opts);
strbuf_release();
return 0;
 fail:
@@ -1034,7 +1039,7 @@ static int pick_commits(struct commit_list *todo_list, 
struct replay_opts *opts)
 * Sequence of picks finished successfully; cleanup by
 * removing the .git/sequencer directory
 */
-   remove_sequencer_state();
+   remove_sequencer_state(opts);
return 0;
 }
 
@@ -1095,7 +1100,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 * one that is being continued
 */
if (opts->subcommand == REPLAY_REMOVE_STATE) {
-   remove_sequencer_state();
+   remove_sequencer_state(opts);
return 0;
}
if (opts->subcommand == REPLAY_ROLLBACK)
-- 
2.10.1.583.g721a9e0

Git context menu in Windows Exporer has a “git bash” option but it doesn't work

[Stefan Monov]

Hi.

The default git distribution for windows contains, besides the CLI
tools, a shell integration, i.e. a context menu for Windows Explorer.
This context menu has a "Git bash" option which I suppose should open
a Git bash prompt cd'd to the current directory. But instead, the git
bash window opens and immediately closes, giving me no chance to even
see what it says inside it. Any fix?

Note: Opening Git Bash from the Windows Start Menu works fine, but if
I open it that way, then I need to manually cd to the needed dir.

TIA,
Stefan Monov

Re: [BUG] fetch output is ugly in 'next'

[Duy Nguyen]

On Fri, Oct 21, 2016 at 7:26 AM, Jeff King  wrote:
> I recently started using lt/abbrev-auto via 'next'. This is the fetch
> output I saw today:
>
> $ git fetch
> remote: Counting objects: 332, done.
> remote: Compressing objects: 100% (237/237), done.
> remote: Total 332 (delta 182), reused 64 (delta 64), pack-reused 31
> Receiving objects: 100% (332/332), 351.53 KiB | 0 bytes/s, done.
> Resolving deltas: 100% (184/184), completed with 26 local objects.
> From git://github.com/gitster/git
>  + fb65135d9c...16ab66ec97 jch  -> origin/jch 
>  (forced update)
>fd47ae6a5b..98985c6911 jk/diff-submodule-diff-inline-> 
> origin/jk/diff-submodule-diff-inline
>  * [new branch]  jk/no-looking-at-dotgit-outside-repo -> 
> origin/jk/no-looking-at-dotgit-outside-repo
>  + 3a8f853f16...1369f9b5ba jt/trailer-with-cruft-> 
> origin/jt/trailer-with-cruft  (forced update)
>  * [new branch]  pt/gitgui-updates-> 
> origin/pt/gitgui-updates
>  + 7594b34cbb...be8e40093b pu   -> origin/pu  
> (forced update)
>  + 7b95cf9a4e...76e368c378 tg/add-chmod+x-fix   -> 
> origin/tg/add-chmod+x-fix  (forced update)
>  + c4cf1f93cf...221912514c va/i18n-perl-scripts -> 
> origin/va/i18n-perl-scripts  (forced update)
>  * [new branch]  yk/git-tag-remove-mention-of-old-layout-in-doc -> 
> origin/yk/git-tag-remove-mention-of-old-layout-in-doc
>
> Yuck.

My eyes!

> I could maybe get over the wasted space due to the longer sha1s,
> but we clearly need to fix the alignment computation. I haven't dug on
> it at all; it's probably low-hanging fruit if somebody is interested.

Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something
sensible should do it.
-- 
Duy

Re: [PATCH v4 20/25] sequencer: refactor write_message()

[Johannes Schindelin]

Hi Junio,

On Thu, 20 Oct 2016, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > If I were doing this, I would make this into three separate steps:
> >
> > - move the strbuf_release(msgbuf) to the caller in
> >   do_pick_commit();
> >
> > - add the missing rollback_lock_file(), which is a bugfix; and
> >   then finally
> >
> > - allow the helper to take not a strbuf but  pair as
> >   parameters.
> >
> > The end result of this patch achieves two thirds of the above, but
> > especially given that write_message() only has two call sites in a
> > single function, I think it is OK and preferrable even to do all
> > three.
> 
> Ah, make that four steps.  The final one is:
> 
> - add append_eol parameter that nobody uses at this step in the
>   series.

Done,
Dscho

Re: [PATCH v4 18/25] sequencer: do not try to commit when there were merge conflicts

[Johannes Schindelin]

Hi Junio,

On Thu, 20 Oct 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > The return value of do_recursive_merge() may be positive (indicating merge
> > conflicts), or 0 (indicating success). It also may be negative, indicating
> > a fatal error that requires us to abort.
> >
> > Now, if the return value indicates that there are merge conflicts, we
> > should not try to commit those changes, of course.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  sequencer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index cbc3742..9ffc090 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -787,7 +787,7 @@ static int do_pick_commit(enum todo_command command, 
> > struct commit *commit,
> > res = allow;
> > goto leave;
> > }
> > -   if (!opts->no_commit)
> > +   if (!res && !opts->no_commit)
> > res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
> >  opts, allow, opts->edit, 0, 0);
> 
> This by itself looks more like a bugfix than preparation for later
> steps.  The only reason why it is not a bugfix is because there is
> nothing in this function that makes res a non-zero value and reach
> this if statement at this step.  We would have been caught by an 
> "if (res) { ... rerere(); goto leave; }" or 
> "if (allow < 0) { res = allow; goto leave; }" 
> that appear before this part of the code.
> 
> So while it is not wrong per-se, I think this should be part of an
> actual change that makes it possible for the control flow to reach
> here with non-zero res.

It looks like it is no longer needed (I *think* that it was made obsolete
by the change where I now "goto fast_forward_edit" only in case there were
no errors).

In any case, the patch's gone now,
Dscho