Re: [PATCH v1] convert: add "status=delayed" to filter process protocol

[Taylor Blau]

On Wed, Jan 11, 2017 at 11:13:00AM +0100, Lars Schneider wrote:
>
> In v1 I implemented a) with the busy-loop problem in mind.
>
> My thinking was this:
>
> If the filter sees at least one filter request twice then the filter knows 
> that
> Git has already requested all files that require filtering. At that point the
> filter could just block the "delayed" answer to the latest filter request 
> until
> at least one of the previously delayed requests can be fulfilled. Then the 
> filter
> answers "delay" to Git until Git requests the blob that can be fulfilled. This
> process cycles until all requests can be fulfilled. Wouldn't that work?
>
> I think a "done" message by the filter is not easy. Right now the protocol 
> works
> in a mode were Git always asks and the filter always answers. I believe 
> changing
> the filter to be able to initiate a "done" message would complicated the 
> protocol.
>
> > Additionally, the protocol should specify a sentinel "no more entries" value
> > that could be sent from Git to the filter to signal that there are no more 
> > files
> > to checkout. Some filters may implement mechanisms for converting files that
> > require a signal to know when all files have been sent. Specifically, Git 
> > LFS
> > (https://git-lfs.github.com) batches files to be transferred together, and 
> > needs
> > to know when all files have been announced to truncate and send the last 
> > batch,
> > if it is not yet full. I'm sure other filter implementations use a similar
> > mechanism and would benefit from this as well.
>
> I agree. I think the filter already has this info implicitly as explained 
> above
> but an explicit message would be better!

This works, but forces some undesirable constraints:

- The filter has to keep track of all items in the checkout to determine how
  many times Git has asked about them. This is probably fine, though annoying.
  Cases where this is not fine is when there are _many_ items in the checkout
  forcing filter implementations to keep track of large sets of data.
- The protocol is now _must_ ask for the status of checkout items in a
  particular order. If the assumption is that "Git has sent me all items in the
  checkout by the point I see Git ask for the status of an item more than once",
  then Git _cannot_ ask for the status of a delayed item while there are still
  more items to checkout. This could be OK, so long as the protocol commits to
  it and documents this behavior.

What are your thoughts?

--
Thanks,
Taylor Blau

ttayl...@github.com

Re: [PATCH v1] convert: add "status=delayed" to filter process protocol

[Taylor Blau]

On Tue, Jan 10, 2017 at 11:11:01PM +0100, Jakub Narębski wrote:
> W dniu 09.01.2017 o 00:42, Junio C Hamano pisze:
> > larsxschnei...@gmail.com writes:
> >> From: Lars Schneider <larsxschnei...@gmail.com>
> >>
> >> Some `clean` / `smudge` filters might require a significant amount of
> >> time to process a single blob. During this process the Git checkout
> >> operation is blocked and Git needs to wait until the filter is done to
> >> continue with the checkout.
>
> Lars, what is expected use case for this feature; that is when do you
> think this problem may happen?  Is it something that happened IRL?
>
> >>
> >> Teach the filter process protocol (introduced in edcc858) to accept the
> >> status "delayed" as response to a filter request. Upon this response Git
> >> continues with the checkout operation and asks the filter to process the
> >> blob again after all other blobs have been processed.
> >
> > Hmm, I would have expected that the basic flow would become
> >
> > for each paths to be processed:
> > convert-to-worktree to buf
> > if not delayed:
> > do the caller's thing to use buf
> > else:
> > remember path
> >
> > for each delayed paths:
> > ensure filter process finished processing for path
> > fetch the thing to buf from the process
> > do the caller's thing to use buf
>
> I would expect here to have a kind of event loop, namely
>
> while there are delayed paths:
> get path that is ready from filter
> fetch the thing to buf (supporting "delayed")
> if path done
> do the caller's thing to use buf
> (e.g. finish checkout path, eof convert, etc.)
>
> We can either trust filter process to tell us when it finished sending
> delayed paths, or keep list of paths that are being delayed in Git.

This makes a lot of sense to me. The "get path that is ready from filter" should
block until the filter has data that it is ready to send. This way Git isn't
wasting time in a busy-loop asking whether the filter has data ready to be sent.
It also means that if the filter has one large chunk that it's ready to write,
Git can work on that while the filter continues to process more data,
theoretically improving the performance of checkouts with many large delayed
objects.

>
> >
> > and that would make quite a lot of sense.  However, what is actually
> > implemented is a bit disappointing from that point of view.  While
> > its first part is the same as above, the latter part instead does:
> >
> > for each delayed paths:
> > checkout the path
> >
> > Presumably, checkout_entry() does the "ensure that the process is
> > done converting" (otherwise the result is simply buggy), but what
> > disappoints me is that this does not allow callers that call
> > "convert-to-working-tree", whose interface is obtain the bytestream
> > in-core in the working tree representation, given an object in the
> > object-db representation in an in-core buffer, to _use_ the result
> > of the conversion.  The caller does not have a chance to even see
> > the result as it is written straight to the filesystem, once it
> > calls checkout_delayed_entries().
> >
>

In addition to the above, I'd also like to investigate adding a "no more items"
message into the filter protocol. This would be useful for filters that
batch delayed items into groups. In particular, if the batch size is `N`, and 
Git
sends `2N-1` items, the second batch will be under-filled. The filter on the
other end needs some mechanism to send the second batch, even though it hasn't
hit max capacity.

Specifically, this is how Git LFS implements object transfers for data it does
not have locally, but I'm sure that this sort of functionality would be useful
for other filter implementations as well.

--
Thanks,
Taylor Blau

ttayl...@github.com

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

[Taylor Blau]

> > (And at this point, may I suggest to change "delay-id" into "request-id=1" ?
>
> If there is no objection by another reviewer then I am happy to change it.

I think "delay-id" may be more illustrative of what's occurring in this request.
That being said, my preference would be that we remove the
"delay-id"/"request-id" entirely from the protocol, and make Git responsible for
handling the path lookup by a hashmap.

Is the concern that a hashmap covering all entries in a large checkout would be
too large to keep in memory? If so, keeping an opaque ID as a part of the
protocol is something I would not object to.

> >> +packet:  git> 
> >> +packet:  git>   # empty content!
> >> +packet:  git< status=success
> >> +packet:  git< 
> >> +packet:  git< SMUDGED_CONTENT
> >> +packet:  git< 
> >> +packet:  git< 
> >
> > OK, good.
> >
> > The quest is: what happens next ?
> >
> > 2 things, kind of in parallel, but we need to prioritize and serialize:
> > - Send the next blob
> > - Fetch ready blobs
> > - And of course: ask for more ready blobs.
> > (it looks as if Peff and Jakub had useful comments already,
> >  so I can stop here?)
>
> I would like to keep the mechanism as follows:
>
> 1. sends all blobs to the filter
> 2. fetch blobs until we are done
>
> @Taylor: Do you think that would be OK for LFS?

I think that this would be fine for LFS and filters of this kind in general. For
LFS in particular, my initial inclination would be to have the protocol open
support writing blob data back to Git at anytime during the checkout process,
not just after all blobs have been sent to the filter.

That being said, I don't think this holds up in practice. The blobs are too big
to fit in memory anyway, and will just end up getting written to LFS's object
cache in .git/lfs/objects.

Since they're already in there, all we would have to do is keep the list of
`readyIds map[int]*os.File` in memory (or even map int -> LFS OID, and open the
file later), and then `io.Copy()` from the open file back to Git.

This makes me think of adding another capability to the protocol, which would
just be exchanging paths on disk in `/tmp` or any other directory so that we
wouldn't have to stream content over the pipe. Instead of responding with

packet:  git< status=success
packet:  git< 
packet:  git< SMUDGED_CONTENT
packet:  git< 
packet:  git< 

We could respond with:

packet:  git< status=success
packet:  git< 
packet:  git< /path/to/contents.dat # <-
packet:  git< 
packet:  git< 

Git would then be responsible for opening that file on disk (the filter would
guarantee that to be possible), and then copying its contents into the working
tree.

I think that's a topic for later discussion, though :-).

> > In general, Git should not have a unlimited number of blobs outstanding,
> > as memory constraints may apply.
> > There may be a config variable for the number of outstanding blobs,
> > (similar to the window size in other protocols) and a variable
> > for the number of "send bytes in outstanding blobs"
> > (similar to window size (again!) in e.g TCP)
> >
> > The number of outstanding blobs is may be less important, and it is more
> > important to monitor the number of bytes we keep in memory in some way.
> >
> > Something like "we set a limit to 500K of out standng data", once we are
> > above the limit, don't send any new blobs.
>
> I don't expect the filter to keep everything in memory. If there is no memory
> anymore then I expect the filter to spool to disk. This keeps the protocol 
> simple.
> If this turns out to be not sufficient then we could improve that later, too.

Agree.


--
Thanks,
Taylor Blau

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

[Taylor Blau]

I think this is a great approach and one that I'd be happy to implement in LFS.
The additional capability isn't too complex, so I think other similar filters to
LFS shouldn't have a hard time implementing it either.

I left a few comments, mostly expressing approval to the documentation changes.
I'll leave the C review to someone more expert than me.

+1 from me on the protocol changes.

> +Delay
> +^
> +
> +If the filter supports the "delay" capability, then Git can send the
> +flag "delay-able" after the filter command and pathname.

Nit: I think either way is fine, but `can_delay` will save us 1 byte per each
new checkout entry.

> +"delay-id", a number that identifies the blob, and a flush packet. The
> +filter acknowledges this number with a "success" status and a flush
> +packet.

I mentioned this in another thread, but I'd prefer, if possible, that we use the
pathname as a unique identifier for referring back to a particular checkout
entry. I think adding an additional identifier adds unnecessary complication to
the protocol and introduces a forced mapping on the filter side from id to
path.

Both Git and the filter are going to have to keep these paths in memory
somewhere, be that in-process, or on disk. That being said, I can see potential
troubles with a large number of long paths that exceed the memory available to
Git or the filter when stored in a hashmap/set.

On Git's side, I think trading that for some CPU time might make sense. If Git
were to SHA1 each path and store that in a hashmap, it would consume more CPU
time, but less memory to store each path. Git and the filter could then exchange
path names, and Git would simply SHA1 the pathname each time it needed to refer
back to memory associated with that entry in a hashmap.

> +by a "success" status that is also terminated with a flush packet. If
> +no blobs for the delayed paths are available, yet, then the filter is
> +expected to block the response until at least one blob becomes
> +available. The filter can tell Git that it has no more delayed blobs
> +by sending an empty list.

I think the blocking nature of the `list_available_blobs` command is a great
synchronization mechanism for telling the filter when it can and can't send new
blob information to Git. I was tenatively thinking of suggesting to remove this
command and instead allow the filter to send readied blobs in sequence after all
unique checkout entries had been sent from Git to the filter. But I think this
allows approach allows us more flexibility, and isn't that much extra
complication or bytes across the pipe.


--
Thanks,
Taylor Blau

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

[Taylor Blau]

On Tue, Apr 18, 2017 at 06:14:36PM +0200, Lars Schneider wrote:
> > Both Git and the filter are going to have to keep these paths in memory
> > somewhere, be that in-process, or on disk. That being said, I can see 
> > potential
> > troubles with a large number of long paths that exceed the memory available 
> > to
> > Git or the filter when stored in a hashmap/set.
> >
> > On Git's side, I think trading that for some CPU time might make sense. If 
> > Git
> > were to SHA1 each path and store that in a hashmap, it would consume more 
> > CPU
> > time, but less memory to store each path. Git and the filter could then 
> > exchange
> > path names, and Git would simply SHA1 the pathname each time it needed to 
> > refer
> > back to memory associated with that entry in a hashmap.
>
> I would be surprised if this would be necessary. If we filter delay 50,000
> files (= a lot!) with a path length of 1000 characters (= very long!) then we
> would use 50MB plus some hashmap data structures. Modern machines should have
> enough RAM I would think...

I agree, and thanks for correcting my thinking here. I ran a simple command to
get the longest path names in a large repository, as:

  $ find . -type f | awk '{ print length($1) }' | sort -r -n | uniq -c

And found a few files close to the 200 character mark as the longest pathnames
in the repository. I think 50k files at 1k bytes per pathname is quite enough
head-room :-).


--
Thanks,
Taylor Blau

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

[Taylor Blau]

I have no remaining concerns about the protocol specification in terms of
implementing a filter with this capability.

Re: [PATCH v2] ref-filter.c: pass empty-string as NULL to atom parsers

[Taylor Blau]

On Tue, Oct 03, 2017 at 08:55:01AM +0900, Junio C Hamano wrote:
> Jonathan Nieder  writes:
>
> > The above does a nice job of explaining
> >
> >  - what this change is going to do
> >  - how it's good for the internal code structure / maintainability
> >
> > What it doesn't tell me about is why the user-facing effect won't
> > cause problems.  Is there no atom where %(atom:) was previously
> > accepted and did something meaningful that this may break?
>
> That is, was there any situation where %(atom) and %(atom:) did two
> differnt things and their differences made sense?
>
> > Looking at the manpage and code, I don't see any, so for what it's
> > worth, this is
> >
> > Reviewed-by: Jonathan Nieder 
> >
> > but for next time, please remember to discuss regression risk in
> > the commit message, too.
>
> Yes, I agree that it is necessary to make sure somebody looked at
> the issue _and_ record the fact that it happened.  Thanks for doing
> that already ;-)
>
> I also took a look at the code and currently we seem to abort,
> either with "unrecognised arg" (e.g. "refname:") or "does not take
> args" (e.g. "body"), so we should be OK, I'd think.

Thank you both for the helpful pointers. I will make sure to include a
more thorough review of potential breakage in existing code given a
particular change.

With respect to this particular patch, I agree with Jonathan and Junio
that there are no places in ref-filter.c that would be affected by this
change, and that it should be able to be applied cleanly without
breakage.

--
- Taylor

[PATCH 0/5] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

Hi,

Attached is a patch to extend Peff's recent work of adding parsing options to
"--pretty=%(trailers)" by supporting those same options in git-for-each-ref(1).

In summary, this patch adds correct behavior for the following options, when
given to git-for-each-ref(1):

  * --format="%(trailers:only)"

  * --format="%(trailers:unfold,unfold)"

  * --format="%(contents:trailers:only)"

  * --format="%(contents:trailers:unfold,unfold)"

I have changed the syntax for specifying multiple sub-arguments in
%(contents:trailers) and %(trailers) atoms to be ","-delimited instead of
%":"-delimited. This is consistent with similar atoms, and is described in
%greater detail in "pretty.c: delimit "%(trailers)" arguments with ","".

I am also new around here: this is my first patch that I am sending to the
mailing list, so this process is entirely new to me. My current focus is helping
maintain Git LFS [1] at GitHub. If I have made any mistakes in formatting these
patches or sending them, please let me know :-).

Thank you in advance.

--
- Taylor

[1]: https://git-lfs.github.com

Taylor Blau (5):
  pretty.c: delimit "%(trailers)" arguments with ","
  t6300: refactor %(trailers) tests
  ref-filter.c: add trailer options to used_atom
  ref-filter.c: use trailer_opts to format trailers
  ref-filter.c: parse trailers arguments with %(contents) atom

 Documentation/git-for-each-ref.txt |  7 ++-
 pretty.c   | 13 +++---
 ref-filter.c   | 38 +++-
 t/t4205-log-pretty-formats.sh  |  4 +-
 t/t6300-for-each-ref.sh| 88 +-
 5 files changed, 129 insertions(+), 21 deletions(-)

--
2.14.1.145.gb3622a4ee

[PATCH 2/5] t6300: refactor %(trailers) tests

[Taylor Blau]

We currently have one test for %(trailers) in `git-for-each-ref(1)`, through
"%(contents:trailers)". In preparation for more, let's add a few things:

  - Move the commit creation step to its own test so that it can be re-used.

  - Add a non-trailer to the commit's trailers to test that non-trailers aren't
shown using "%(trailers:only)".

  - Add a multi-line trailer to ensure that trailers are unfolded correctly
using "%(trailers:unfold)".

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 t/t6300-for-each-ref.sh | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 834a9ed16..2a9fcf713 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -592,18 +592,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) 
on unborn branch' '
 cat >trailers <
 Signed-off-by: A U Thor <aut...@example.com>
+[ v2 updated patch description ]
+Acked-by: A U Thor
+  <aut...@example.com>
 EOF
 
-test_expect_success 'basic atom: head contents:trailers' '
+
+test_expect_success 'set up trailers for next test' '
echo "Some contents" > two &&
git add two &&
-   git commit -F - <<-EOF &&
+   git commit -F - <<-EOF
trailers: this commit message has trailers
 
Some message contents
 
$(cat trailers)
EOF
+'
+
+test_expect_success 'basic atom: head contents:trailers' '
git for-each-ref --format="%(contents:trailers)" refs/heads/master 
>actual &&
sanitize_pgp actual.clean &&
# git for-each-ref ends with a blank line
-- 
2.14.1.145.gb3622a4ee

[PATCH 1/5] pretty.c: delimit "%(trailers)" arguments with ","

[Taylor Blau]

In preparation for adding consistent "%(trailers)" atom options to
`git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in pretty.c
to separate sub-arguments with a ",", instead of a ":".

Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
"%(trailers:only,unfold)".

This change disambiguates between "top-level" arguments, and arguments given to
the trailers atom itself. It is consistent with the behavior of "%(upstream)"
and "%(push)" atoms.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 pretty.c  | 13 -
 t/t4205-log-pretty-formats.sh |  4 ++--
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/pretty.c b/pretty.c
index 0e23fe3c0..68f736912 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1265,11 +1265,14 @@ static size_t format_commit_one(struct strbuf *sb, /* 
in UTF-8 */
 
if (skip_prefix(placeholder, "(trailers", )) {
struct process_trailer_options opts = 
PROCESS_TRAILER_OPTIONS_INIT;
-   while (*arg == ':') {
-   if (skip_prefix(arg, ":only", ))
-   opts.only_trailers = 1;
-   else if (skip_prefix(arg, ":unfold", ))
-   opts.unfold = 1;
+   if (skip_prefix(arg, ":", )) {
+   while (*arg != ')') {
+   skip_prefix(arg, ",", );
+   if (skip_prefix(arg, "only", ))
+   opts.only_trailers = 1;
+   else if (skip_prefix(arg, "unfold", ))
+   opts.unfold = 1;
+   }
}
if (*arg == ')') {
format_trailers_from_commit(sb, msg + c->subject_off, 
);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f53010..977472f53 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 '
 
 test_expect_success ':only and :unfold work together' '
-   git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
-   git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+   git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
+   git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
test_cmp actual reverse &&
{
grep -v patch.description 

[PATCH v3 1/6] pretty.c: delimit "%(trailers)" arguments with ","

[Taylor Blau]

In preparation for adding consistent "%(trailers)" atom options to
`git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in
pretty.c to separate sub-arguments with a ",", instead of a ":".

Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
"%(trailers:only,unfold)".

This change disambiguates between "top-level" arguments, and arguments
given to the trailers atom itself. It is consistent with the behavior of
"%(upstream)" and "%(push)" atoms.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 pretty.c  | 34 +-
 t/t4205-log-pretty-formats.sh |  4 ++--
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/pretty.c b/pretty.c
index 0e23fe3c0..9f8d75a84 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1036,6 +1036,25 @@ static size_t parse_padding_placeholder(struct strbuf 
*sb,
return 0;
 }
 
+static int match_placeholder_arg(const char *to_parse,
+   const char *candidate,
+   const char **end)
+{
+   const char *p;
+   if (!(skip_prefix(to_parse, candidate, )))
+   return 0;
+   if (*p == ',') {
+   *end = p + 1;
+   return 1;
+   }
+   if (*p == ')') {
+   *end = p;
+   return 1;
+   }
+   return 0;
+}
+
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
void *context)
@@ -1265,11 +1284,16 @@ static size_t format_commit_one(struct strbuf *sb, /* 
in UTF-8 */
 
if (skip_prefix(placeholder, "(trailers", )) {
struct process_trailer_options opts = 
PROCESS_TRAILER_OPTIONS_INIT;
-   while (*arg == ':') {
-   if (skip_prefix(arg, ":only", ))
-   opts.only_trailers = 1;
-   else if (skip_prefix(arg, ":unfold", ))
-   opts.unfold = 1;
+   if (*arg == ':') {
+   arg++;
+   for (;;) {
+   if (match_placeholder_arg(arg, "only", ))
+   opts.only_trailers = 1;
+   else if (match_placeholder_arg(arg, "unfold", 
))
+   opts.unfold = 1;
+   else
+   break;
+   }
}
if (*arg == ')') {
format_trailers_from_commit(sb, msg + c->subject_off, 
);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f53010..977472f53 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 '
 
 test_expect_success ':only and :unfold work together' '
-   git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
-   git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+   git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
+   git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
test_cmp actual reverse &&
{
grep -v patch.description 

[PATCH v3 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

The %(contents) atom takes a contents "field" as its argument. Since
"trailers" is one of those fields, extend contents_atom_parser to parse
"trailers"'s arguments when used through "%(contents)", like:

  %(contents:trailers:unfold,only)

A caveat: trailers_atom_parser expects NULL when no arguments are given
(see: `parse_ref_filter_atom`). To simulate this behavior without
teaching trailers_atom_parser to accept strings with length zero,
conditionally pass NULL to trailers_atom_parser if the arguments portion
of the argument to %(contents) is empty.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 ref-filter.c|  6 --
 t/t6300-for-each-ref.sh | 37 +
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8573acbed..b5747e969 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -207,8 +207,10 @@ static void contents_atom_parser(struct used_atom *atom, 
const char *arg)
atom->u.contents.option = C_SIG;
else if (!strcmp(arg, "subject"))
atom->u.contents.option = C_SUB;
-   else if (!strcmp(arg, "trailers"))
-   atom->u.contents.option = C_TRAILERS;
+   else if (skip_prefix(arg, "trailers", )) {
+   skip_prefix(arg, ":", );
+   trailers_atom_parser(atom, *arg ? NULL : arg);
+   }
else if (skip_prefix(arg, "lines=", )) {
atom->u.contents.option = C_LINES;
if (strtoul_ui(arg, 10, >u.contents.nlines))
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 6538e7b90..8c960ec24 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -642,6 +642,35 @@ test_expect_success '%(trailers:only) and 
%(trailers:unfold) work together' '
test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
+   git for-each-ref --format="%(contents:trailers:unfold)" 
refs/heads/master >actual &&
+   {
+   unfold expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) shows only "key: value" 
trailers' '
+   git for-each-ref --format="%(contents:trailers:only)" refs/heads/master 
>actual &&
+   {
+   grep -v patch.description expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) 
work together' '
+   git for-each-ref --format="%(contents:trailers:only,unfold)" 
refs/heads/master >actual &&
+   git for-each-ref --format="%(contents:trailers:unfold,only)" 
refs/heads/master >reverse &&
+   test_cmp actual reverse &&
+   {
+   grep -v patch.description expect &&
+   test_cmp expect actual
+'
+
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
cat >expect <<-EOF &&
fatal: unknown %(trailers) argument: unsupported
@@ -650,6 +679,14 @@ test_expect_success '%(trailers) rejects unknown trailers 
arguments' '
   test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
+   cat >expect <<-EOF &&
+   fatal: unknown %(trailers) argument: unsupported
+   EOF
+  test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 
2>actual &&
+  test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
git for-each-ref --format="%(contents:trailers)" refs/heads/master 
>actual &&
sanitize_pgp actual.clean &&
-- 
2.14.1.145.gb3622a4ee

[PATCH v3 5/6] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

Fill trailer_opts with "unfold" and "only" to match the sub-arguments
given to the "%(trailers)" atom. Then, let's use the filled trailer_opts
instance with 'format_trailers_from_commit' in order to format trailers
in the desired manner.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  5 -
 ref-filter.c   | 32 +-
 t/t6300-for-each-ref.sh| 40 ++
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index b6492820b..a1d964182 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -214,7 +214,10 @@ blank line.  The optional GPG signature is 
`contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
 are obtained as `trailers` (or by using the historical alias
-`contents:trailers`).
+`contents:trailers`).  Non-trailer lines from the trailer block can be omitted
+with `trailers:only`. Whitespace-continuations can be removed from trailers so
+that each trailer appears on a line by itself with its full content with
+`trailers:unfold`. Both can be used together as `trailers:unfold,only`.
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index 467c0279c..8573acbed 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -82,6 +82,7 @@ static struct used_atom {
} remote_ref;
struct {
enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
C_SUB, C_TRAILERS } option;
+   struct process_trailer_options trailer_opts;
unsigned int nlines;
} contents;
struct {
@@ -177,9 +178,23 @@ static void subject_atom_parser(struct used_atom *atom, 
const char *arg)
 
 static void trailers_atom_parser(struct used_atom *atom, const char *arg)
 {
-   if (arg)
-   die(_("%%(trailers) does not take arguments"));
+   struct string_list params = STRING_LIST_INIT_DUP;
+   int i;
+
+   if (arg) {
+   string_list_split(, arg, ',', -1);
+   for (i = 0; i < params.nr; i++) {
+   const char *s = params.items[i].string;
+   if (!strcmp(s, "unfold"))
+   atom->u.contents.trailer_opts.unfold = 1;
+   else if (!strcmp(s, "only"))
+   atom->u.contents.trailer_opts.only_trailers = 1;
+   else
+   die(_("unknown %%(trailers) argument: %s"), s);
+   }
+   }
atom->u.contents.option = C_TRAILERS;
+   string_list_clear(, 0);
 }
 
 static void contents_atom_parser(struct used_atom *atom, const char *arg)
@@ -1034,7 +1049,7 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
name++;
if (strcmp(name, "subject") &&
strcmp(name, "body") &&
-   strcmp(name, "trailers") &&
+   !starts_with(name, "trailers") &&
!starts_with(name, "contents"))
continue;
if (!subpos)
@@ -1059,13 +1074,12 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
append_lines(, subpos, contents_end - subpos, 
atom->u.contents.nlines);
v->s = strbuf_detach(, NULL);
} else if (atom->u.contents.option == C_TRAILERS) {
-   struct trailer_info info;
+   struct strbuf s = STRBUF_INIT;
 
-   /* Search for trailer info */
-   trailer_info_get(, subpos);
-   v->s = xmemdupz(info.trailer_start,
-   info.trailer_end - info.trailer_start);
-   trailer_info_release();
+   /* Format the trailer info according to the 
trailer_opts given */
+   format_trailers_from_commit(, subpos, 
>u.contents.trailer_opts);
+
+   v->s = strbuf_detach(, NULL);
} else if (atom->u.contents.option == C_BARE)
v->s = xstrdup(subpos);
}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2a9fcf713..6538e7b90 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -597,6 +597,9 @@ Acked-by: A U T

[PATCH v3 4/6] doc: use modern "`"-style code fencing

[Taylor Blau]

"'"- (single-quote) styled code fencing is no longer considered modern
within the "Documentation/" subtree.

In preparation for adding additional information to this section of
git-for-each-ref(1)'s documentation, update old-style code fencing to
use "`"-style fencing instead.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 6b38d9a22..b6492820b 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -209,12 +209,12 @@ and `date` to extract the named component.
 The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 of all lines of the commit message up to the first blank line.  The next
-line is 'contents:body', where body is all of the lines after the first
+line is `contents:body`, where body is all of the lines after the first
 blank line.  The optional GPG signature is `contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
-are obtained as 'trailers' (or by using the historical alias
-'contents:trailers').
+are obtained as `trailers` (or by using the historical alias
+`contents:trailers`).
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v3 2/6] t6300: refactor %(trailers) tests

[Taylor Blau]

We currently have one test for %(trailers) in `git-for-each-ref(1)`,
through "%(contents:trailers)". In preparation for more, let's add a few
things:

  - Move the commit creation step to its own test so that it can be
  re-used.

  - Add a non-trailer to the commit's trailers to test that non-trailers
  aren't shown using "%(trailers:only)".

  - Add a multi-line trailer to ensure that trailers are unfolded
  correctly using "%(trailers:unfold)".

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 t/t6300-for-each-ref.sh | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 834a9ed16..2a9fcf713 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -592,18 +592,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) 
on unborn branch' '
 cat >trailers <
 Signed-off-by: A U Thor <aut...@example.com>
+[ v2 updated patch description ]
+Acked-by: A U Thor
+  <aut...@example.com>
 EOF
 
-test_expect_success 'basic atom: head contents:trailers' '
+
+test_expect_success 'set up trailers for next test' '
echo "Some contents" > two &&
git add two &&
-   git commit -F - <<-EOF &&
+   git commit -F - <<-EOF
trailers: this commit message has trailers
 
Some message contents
 
$(cat trailers)
EOF
+'
+
+test_expect_success 'basic atom: head contents:trailers' '
git for-each-ref --format="%(contents:trailers)" refs/heads/master 
>actual &&
sanitize_pgp actual.clean &&
# git for-each-ref ends with a blank line
-- 
2.14.1.145.gb3622a4ee

[PATCH v3 3/6] doc: 'trailers' is the preferred way to format trailers

[Taylor Blau]

The documentation makes reference to 'contents:trailers' as an example
to dig the trailers out of a commit. 'trailers' is an unmentioned
alternative, which is treated as an alias of 'contents:trailers'.

Since 'trailers' is easier to type, prefer that as the designated way to
dig out trailers information.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 03e187a10..6b38d9a22 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -213,7 +213,8 @@ line is 'contents:body', where body is all of the lines 
after the first
 blank line.  The optional GPG signature is `contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
-are obtained as 'contents:trailers'.
+are obtained as 'trailers' (or by using the historical alias
+'contents:trailers').
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH 5/5] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

The %(contents) atom takes a contents "field" as its argument. Since "trailers"
is one of those fields, extend contents_atom_parser to parse "trailers"'s
arguments when used through "%(contents)", like:

  %(contents:trailers:unfold,only)

A caveat: trailers_atom_parser expects NULL when no arguments are given (see:
`parse_ref_filter_atom`). To simulate this behavior without teaching
trailers_atom_parser to accept strings with length zero, conditionally pass
NULL to trailers_atom_parser if the arguments portion of the argument to
%(contents) is empty.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  9 +
 ref-filter.c   |  6 --
 t/t6300-for-each-ref.sh| 37 +
 3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index b7325a25d..0aaac8af9 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -214,10 +214,11 @@ blank line.  The optional GPG signature is 
`contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
 are obtained as 'contents:trailers'. Non-trailer lines from the trailer block
-can be omitted with 'trailers:only'. Whitespace-continuations can be removed
-from trailers so that each trailer appears on a line by itself with its full
-content with 'trailers:unfold'. Both can be used together as
-'trailers:unfold,only'.
+can be omitted with 'trailers:only', or 'contents:trailers:only'.
+Whitespace-continuations can be removed from trailers so that each trailer
+appears on a line by itself with its full content with 'trailers:unfold' or
+'contents:trailers:unfold'. Both can be used together as 
'trailers:unfold,only',
+or 'contents:trailers:unfold,only'.
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index 8573acbed..a8d4a52bd 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -207,8 +207,10 @@ static void contents_atom_parser(struct used_atom *atom, 
const char *arg)
atom->u.contents.option = C_SIG;
else if (!strcmp(arg, "subject"))
atom->u.contents.option = C_SUB;
-   else if (!strcmp(arg, "trailers"))
-   atom->u.contents.option = C_TRAILERS;
+   else if (skip_prefix(arg, "trailers", )) {
+   skip_prefix(arg, ":", );
+   trailers_atom_parser(atom, strlen(arg) ? arg : NULL);
+   }
else if (skip_prefix(arg, "lines=", )) {
atom->u.contents.option = C_LINES;
if (strtoul_ui(arg, 10, >u.contents.nlines))
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2bd0c5da7..d9b71589f 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -642,6 +642,35 @@ test_expect_success '%(trailers:only) and 
%(trailers:unfold) work together' '
test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
+  git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master 
>actual &&
+  {
+unfold expect &&
+  test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) shows only "key: value" 
trailers' '
+   git for-each-ref --format="%(contents:trailers:only)" refs/heads/master 
>actual &&
+   {
+   grep -v patch.description expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) 
work together' '
+   git for-each-ref --format="%(contents:trailers:only,unfold)" 
refs/heads/master >actual &&
+   git for-each-ref --format="%(contents:trailers:unfold,only)" 
refs/heads/master >reverse &&
+   test_cmp actual reverse &&
+   {
+   grep -v patch.description expect &&
+   test_cmp expect actual
+'
+
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
cat >expect <<-EOF &&
fatal: unknown %(trailers) argument: unsupported
@@ -650,6 +679,14 @@ test_expect_success '%(trailers) rejects unknown trailers 
arguments' '
   test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
+   cat >expect <<-EOF &&
+   fatal: unknown %(trailers) argument: unsupported
+   EOF
+  test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 
2>actual &&
+  test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
git for-each-ref --format="%(contents:trailers)" refs/heads/master 
>actual &&
sanitize_pgp actual.clean &&
-- 
2.14.1.145.gb3622a4ee

[PATCH 3/5] ref-filter.c: add trailer options to used_atom

[Taylor Blau]

In preparation for "%(trailers)" to take trailer parsing arguments, use Jeff's
convenience structure for trailer processing options introduced in 8abc89800c.

We will later populate this field from the arguments given to %(trailers), and
then use the trailer_opts instance to format ref trailers correctly using
`format_trailer_from_commit`.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 ref-filter.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ref-filter.c b/ref-filter.c
index 467c0279c..84f14093c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -82,6 +82,7 @@ static struct used_atom {
} remote_ref;
struct {
enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
C_SUB, C_TRAILERS } option;
+   struct process_trailer_options trailer_opts;
unsigned int nlines;
} contents;
struct {
-- 
2.14.1.145.gb3622a4ee

[PATCH 4/5] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

In preparation to support additional sub-arguments given to the "%(trailers)"
atom, use 'format_trailers_from_commit' in order to format trailers in the
desired manner.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  6 +-
 ref-filter.c   | 31 -
 t/t6300-for-each-ref.sh| 40 ++
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 03e187a10..b7325a25d 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -213,7 +213,11 @@ line is 'contents:body', where body is all of the lines 
after the first
 blank line.  The optional GPG signature is `contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
-are obtained as 'contents:trailers'.
+are obtained as 'contents:trailers'. Non-trailer lines from the trailer block
+can be omitted with 'trailers:only'. Whitespace-continuations can be removed
+from trailers so that each trailer appears on a line by itself with its full
+content with 'trailers:unfold'. Both can be used together as
+'trailers:unfold,only'.
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index 84f14093c..8573acbed 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -178,9 +178,23 @@ static void subject_atom_parser(struct used_atom *atom, 
const char *arg)
 
 static void trailers_atom_parser(struct used_atom *atom, const char *arg)
 {
-   if (arg)
-   die(_("%%(trailers) does not take arguments"));
+   struct string_list params = STRING_LIST_INIT_DUP;
+   int i;
+
+   if (arg) {
+   string_list_split(, arg, ',', -1);
+   for (i = 0; i < params.nr; i++) {
+   const char *s = params.items[i].string;
+   if (!strcmp(s, "unfold"))
+   atom->u.contents.trailer_opts.unfold = 1;
+   else if (!strcmp(s, "only"))
+   atom->u.contents.trailer_opts.only_trailers = 1;
+   else
+   die(_("unknown %%(trailers) argument: %s"), s);
+   }
+   }
atom->u.contents.option = C_TRAILERS;
+   string_list_clear(, 0);
 }
 
 static void contents_atom_parser(struct used_atom *atom, const char *arg)
@@ -1035,7 +1049,7 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
name++;
if (strcmp(name, "subject") &&
strcmp(name, "body") &&
-   strcmp(name, "trailers") &&
+   !starts_with(name, "trailers") &&
!starts_with(name, "contents"))
continue;
if (!subpos)
@@ -1060,13 +1074,12 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
append_lines(, subpos, contents_end - subpos, 
atom->u.contents.nlines);
v->s = strbuf_detach(, NULL);
} else if (atom->u.contents.option == C_TRAILERS) {
-   struct trailer_info info;
+   struct strbuf s = STRBUF_INIT;
 
-   /* Search for trailer info */
-   trailer_info_get(, subpos);
-   v->s = xmemdupz(info.trailer_start,
-   info.trailer_end - info.trailer_start);
-   trailer_info_release();
+   /* Format the trailer info according to the 
trailer_opts given */
+   format_trailers_from_commit(, subpos, 
>u.contents.trailer_opts);
+
+   v->s = strbuf_detach(, NULL);
} else if (atom->u.contents.option == C_BARE)
v->s = xstrdup(subpos);
}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2a9fcf713..2bd0c5da7 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -597,6 +597,9 @@ Acked-by: A U Thor
   <aut...@example.com>
 EOF
 
+unfold () {
+   perl -0pe 's/\n\s+/ /'
+}
 
 test_expect_success 'set up trailers for next test' '
echo "Some contents" > two &&
@@ -610,6 +613,43 @@ test_expect_success 'set up trailers for next test' '
EOF
 '
 
+test_expect_success '%(trailers:unfold) unfolds trailers' '
+  git for-each-ref --format="%(trailers:unfold)" r

Re: [PATCH v2 0/6] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

Hi,

Attached is v3 of my patch-set "Support %(trailers) arguments in
for-each-ref(1)".

It includes all of the changes in V2:
>   * Accept "empty-sub-argument" lists, like %(contents:trailers:).
>
>   * Fix incorrect tabs/spaces formatting in t6300.
>
>   * Modern-ize code fencing in Documentation/git-for-each-ref.txt
>
>   * Squash commit adding trailer_opts to used_atom structure.

As well as fixing:

  * Incorrect indentation in pretty.c (specifically: "pretty.c: delimit
"%(trailers)" arguments with ","")

  * Issues with --format="%(contents:trailers:)", where
trailers_atom_parser did not correctly handle empty string ("") and
NULL's the same way.

Thanks in advance :-).

--
- Taylor

[PATCH v2 0/6] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

Hi,

Attached is v2 of my patch-set "Support %(trailers) arguments in
for-each-ref(1)".

It includes the following changes since v1:

  * Accept "empty-sub-argument" lists, like %(contents:trailers:).

  * Fix incorrect tabs/spaces formatting in t6300.

  * Modern-ize code fencing in Documentation/git-for-each-ref.txt

  * Squash commit adding trailer_opts to used_atom structure.

---

  [1/6]: pretty.c: delimit "%(trailers)" arguments with ","
  [2/6]: t6300: refactor %(trailers) tests
  [3/6]: doc: 'trailers' is the preferred way to format trailers
  [4/6]: doc: use modern "`"-style code fencing
  [5/6]: ref-filter.c: use trailer_opts to format trailers
  [6/6]: ref-filter.c: parse trailers arguments with %(contents) atom

Thanks in advance :-).

--
- Taylor

[PATCH v2 1/6] pretty.c: delimit "%(trailers)" arguments with ","

[Taylor Blau]

In preparation for adding consistent "%(trailers)" atom options to
`git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in
pretty.c to separate sub-arguments with a ",", instead of a ":".

Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
"%(trailers:only,unfold)".

This change disambiguates between "top-level" arguments, and arguments
given to the trailers atom itself. It is consistent with the behavior of
"%(upstream)" and "%(push)" atoms.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 pretty.c  | 34 +-
 t/t4205-log-pretty-formats.sh |  4 ++--
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/pretty.c b/pretty.c
index 0e23fe3c0..eeb51746c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1036,6 +1036,25 @@ static size_t parse_padding_placeholder(struct strbuf 
*sb,
return 0;
 }
 
+static int match_placeholder_arg(const char *to_parse,
+   
const char *candidate,
+   
const char **end)
+{
+   const char *p;
+   if (!(skip_prefix(to_parse, candidate, )))
+   return 0;
+   if (*p == ',') {
+   *end = p + 1;
+   return 1;
+   }
+   if (*p == ')') {
+   *end = p;
+   return 1;
+   }
+   return 0;
+}
+
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
void *context)
@@ -1265,11 +1284,16 @@ static size_t format_commit_one(struct strbuf *sb, /* 
in UTF-8 */
 
if (skip_prefix(placeholder, "(trailers", )) {
struct process_trailer_options opts = 
PROCESS_TRAILER_OPTIONS_INIT;
-   while (*arg == ':') {
-   if (skip_prefix(arg, ":only", ))
-   opts.only_trailers = 1;
-   else if (skip_prefix(arg, ":unfold", ))
-   opts.unfold = 1;
+   if (*arg == ':') {
+   arg++;
+   for (;;) {
+   if (match_placeholder_arg(arg, "only", ))
+   opts.only_trailers = 1;
+   else if (match_placeholder_arg(arg, "unfold", 
))
+   opts.unfold = 1;
+   else
+   break;
+   }
}
if (*arg == ')') {
format_trailers_from_commit(sb, msg + c->subject_off, 
);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f53010..977472f53 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 '
 
 test_expect_success ':only and :unfold work together' '
-   git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
-   git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+   git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
+   git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
test_cmp actual reverse &&
{
grep -v patch.description 

[PATCH v2 3/6] doc: 'trailers' is the preferred way to format trailers

[Taylor Blau]

The documentation makes reference to 'contents:trailers' as an example
to dig the trailers out of a commit. 'trailers' is an unmentioned
alternative, which is treated as an alias of 'contents:trailers'.

Since 'trailers' is easier to type, prefer that as the designated way to
dig out trailers information.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 03e187a10..6b38d9a22 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -213,7 +213,8 @@ line is 'contents:body', where body is all of the lines 
after the first
 blank line.  The optional GPG signature is `contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
-are obtained as 'contents:trailers'.
+are obtained as 'trailers' (or by using the historical alias
+'contents:trailers').
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v2 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

The %(contents) atom takes a contents "field" as its argument. Since
"trailers" is one of those fields, extend contents_atom_parser to parse
"trailers"'s arguments when used through "%(contents)", like:

  %(contents:trailers:unfold,only)

A caveat: trailers_atom_parser expects NULL when no arguments are given
(see: `parse_ref_filter_atom`). To simulate this behavior without
teaching trailers_atom_parser to accept strings with length zero,
conditionally pass NULL to trailers_atom_parser if the arguments portion
of the argument to %(contents) is empty.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 ref-filter.c|  6 --
 t/t6300-for-each-ref.sh | 37 +
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8573acbed..f5bde79f3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -207,8 +207,10 @@ static void contents_atom_parser(struct used_atom *atom, 
const char *arg)
atom->u.contents.option = C_SIG;
else if (!strcmp(arg, "subject"))
atom->u.contents.option = C_SUB;
-   else if (!strcmp(arg, "trailers"))
-   atom->u.contents.option = C_TRAILERS;
+   else if (skip_prefix(arg, "trailers", )) {
+   skip_prefix(arg, ":", );
+   trailers_atom_parser(atom, arg);
+   }
else if (skip_prefix(arg, "lines=", )) {
atom->u.contents.option = C_LINES;
if (strtoul_ui(arg, 10, >u.contents.nlines))
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 6538e7b90..8c960ec24 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -642,6 +642,35 @@ test_expect_success '%(trailers:only) and 
%(trailers:unfold) work together' '
test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
+   git for-each-ref --format="%(contents:trailers:unfold)" 
refs/heads/master >actual &&
+   {
+   unfold expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) shows only "key: value" 
trailers' '
+   git for-each-ref --format="%(contents:trailers:only)" refs/heads/master 
>actual &&
+   {
+   grep -v patch.description expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) 
work together' '
+   git for-each-ref --format="%(contents:trailers:only,unfold)" 
refs/heads/master >actual &&
+   git for-each-ref --format="%(contents:trailers:unfold,only)" 
refs/heads/master >reverse &&
+   test_cmp actual reverse &&
+   {
+   grep -v patch.description expect &&
+   test_cmp expect actual
+'
+
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
cat >expect <<-EOF &&
fatal: unknown %(trailers) argument: unsupported
@@ -650,6 +679,14 @@ test_expect_success '%(trailers) rejects unknown trailers 
arguments' '
   test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
+   cat >expect <<-EOF &&
+   fatal: unknown %(trailers) argument: unsupported
+   EOF
+  test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 
2>actual &&
+  test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
git for-each-ref --format="%(contents:trailers)" refs/heads/master 
>actual &&
sanitize_pgp actual.clean &&
-- 
2.14.1.145.gb3622a4ee

[PATCH v2 5/6] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

Fill trailer_opts with "unfold" and "only" to match the sub-arguments
given to the "%(trailers)" atom. Then, let's use the filled trailer_opts
instance with 'format_trailers_from_commit' in order to format trailers
in the desired manner.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  5 -
 ref-filter.c   | 32 +-
 t/t6300-for-each-ref.sh| 40 ++
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index b6492820b..a1d964182 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -214,7 +214,10 @@ blank line.  The optional GPG signature is 
`contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
 are obtained as `trailers` (or by using the historical alias
-`contents:trailers`).
+`contents:trailers`).  Non-trailer lines from the trailer block can be omitted
+with `trailers:only`. Whitespace-continuations can be removed from trailers so
+that each trailer appears on a line by itself with its full content with
+`trailers:unfold`. Both can be used together as `trailers:unfold,only`.
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index 467c0279c..8573acbed 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -82,6 +82,7 @@ static struct used_atom {
} remote_ref;
struct {
enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
C_SUB, C_TRAILERS } option;
+   struct process_trailer_options trailer_opts;
unsigned int nlines;
} contents;
struct {
@@ -177,9 +178,23 @@ static void subject_atom_parser(struct used_atom *atom, 
const char *arg)
 
 static void trailers_atom_parser(struct used_atom *atom, const char *arg)
 {
-   if (arg)
-   die(_("%%(trailers) does not take arguments"));
+   struct string_list params = STRING_LIST_INIT_DUP;
+   int i;
+
+   if (arg) {
+   string_list_split(, arg, ',', -1);
+   for (i = 0; i < params.nr; i++) {
+   const char *s = params.items[i].string;
+   if (!strcmp(s, "unfold"))
+   atom->u.contents.trailer_opts.unfold = 1;
+   else if (!strcmp(s, "only"))
+   atom->u.contents.trailer_opts.only_trailers = 1;
+   else
+   die(_("unknown %%(trailers) argument: %s"), s);
+   }
+   }
atom->u.contents.option = C_TRAILERS;
+   string_list_clear(, 0);
 }
 
 static void contents_atom_parser(struct used_atom *atom, const char *arg)
@@ -1034,7 +1049,7 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
name++;
if (strcmp(name, "subject") &&
strcmp(name, "body") &&
-   strcmp(name, "trailers") &&
+   !starts_with(name, "trailers") &&
!starts_with(name, "contents"))
continue;
if (!subpos)
@@ -1059,13 +1074,12 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
append_lines(, subpos, contents_end - subpos, 
atom->u.contents.nlines);
v->s = strbuf_detach(, NULL);
} else if (atom->u.contents.option == C_TRAILERS) {
-   struct trailer_info info;
+   struct strbuf s = STRBUF_INIT;
 
-   /* Search for trailer info */
-   trailer_info_get(, subpos);
-   v->s = xmemdupz(info.trailer_start,
-   info.trailer_end - info.trailer_start);
-   trailer_info_release();
+   /* Format the trailer info according to the 
trailer_opts given */
+   format_trailers_from_commit(, subpos, 
>u.contents.trailer_opts);
+
+   v->s = strbuf_detach(, NULL);
} else if (atom->u.contents.option == C_BARE)
v->s = xstrdup(subpos);
}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2a9fcf713..6538e7b90 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -597,6 +597,9 @@ Acked-by: A U T

[PATCH v2 2/6] t6300: refactor %(trailers) tests

[Taylor Blau]

We currently have one test for %(trailers) in `git-for-each-ref(1)`,
through "%(contents:trailers)". In preparation for more, let's add a few
things:

  - Move the commit creation step to its own test so that it can be
  re-used.

  - Add a non-trailer to the commit's trailers to test that non-trailers
  aren't shown using "%(trailers:only)".

  - Add a multi-line trailer to ensure that trailers are unfolded
  correctly using "%(trailers:unfold)".

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 t/t6300-for-each-ref.sh | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 834a9ed16..2a9fcf713 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -592,18 +592,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) 
on unborn branch' '
 cat >trailers <
 Signed-off-by: A U Thor <aut...@example.com>
+[ v2 updated patch description ]
+Acked-by: A U Thor
+  <aut...@example.com>
 EOF
 
-test_expect_success 'basic atom: head contents:trailers' '
+
+test_expect_success 'set up trailers for next test' '
echo "Some contents" > two &&
git add two &&
-   git commit -F - <<-EOF &&
+   git commit -F - <<-EOF
trailers: this commit message has trailers
 
Some message contents
 
$(cat trailers)
EOF
+'
+
+test_expect_success 'basic atom: head contents:trailers' '
git for-each-ref --format="%(contents:trailers)" refs/heads/master 
>actual &&
sanitize_pgp actual.clean &&
# git for-each-ref ends with a blank line
-- 
2.14.1.145.gb3622a4ee

[PATCH v2 4/6] doc: use modern "`"-style code fencing

[Taylor Blau]

"'"- (single-quote) styled code fencing is no longer considered modern
within the "Documentation/" subtree.

In preparation for adding additional information to this section of
git-for-each-ref(1)'s documentation, update old-style code fencing to
use "`"-style fencing instead.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 6b38d9a22..b6492820b 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -209,12 +209,12 @@ and `date` to extract the named component.
 The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 of all lines of the commit message up to the first blank line.  The next
-line is 'contents:body', where body is all of the lines after the first
+line is `contents:body`, where body is all of the lines after the first
 blank line.  The optional GPG signature is `contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
-are obtained as 'trailers' (or by using the historical alias
-'contents:trailers').
+are obtained as `trailers` (or by using the historical alias
+`contents:trailers`).
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with ","

[Taylor Blau]

In preparation for adding consistent "%(trailers)" atom options to
`git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in
pretty.c to separate sub-arguments with a ",", instead of a ":".

Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
"%(trailers:only,unfold)".

This change disambiguates between "top-level" arguments, and arguments
given to the trailers atom itself. It is consistent with the behavior of
"%(upstream)" and "%(push)" atoms.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 pretty.c  | 34 +-
 t/t4205-log-pretty-formats.sh |  4 ++--
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/pretty.c b/pretty.c
index 94eab5c89..eec128bc1 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1056,6 +1056,25 @@ static size_t parse_padding_placeholder(struct strbuf 
*sb,
return 0;
 }
 
+static int match_placeholder_arg(const char *to_parse,
+   const char *candidate,
+   const char **end)
+{
+   const char *p;
+   if (!(skip_prefix(to_parse, candidate, )))
+   return 0;
+   if (*p == ',') {
+   *end = p + 1;
+   return 1;
+   }
+   if (*p == ')') {
+   *end = p;
+   return 1;
+   }
+   return 0;
+}
+
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
void *context)
@@ -1285,11 +1304,16 @@ static size_t format_commit_one(struct strbuf *sb, /* 
in UTF-8 */
 
if (skip_prefix(placeholder, "(trailers", )) {
struct process_trailer_options opts = 
PROCESS_TRAILER_OPTIONS_INIT;
-   while (*arg == ':') {
-   if (skip_prefix(arg, ":only", ))
-   opts.only_trailers = 1;
-   else if (skip_prefix(arg, ":unfold", ))
-   opts.unfold = 1;
+   if (*arg == ':') {
+   arg++;
+   for (;;) {
+   if (match_placeholder_arg(arg, "only", ))
+   opts.only_trailers = 1;
+   else if (match_placeholder_arg(arg, "unfold", 
))
+   opts.unfold = 1;
+   else
+   break;
+   }
}
if (*arg == ')') {
format_trailers_from_commit(sb, msg + c->subject_off, 
);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f53010..977472f53 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 '
 
 test_expect_success ':only and :unfold work together' '
-   git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
-   git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+   git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
+   git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
test_cmp actual reverse &&
{
grep -v patch.description 

[PATCH v4 5/6] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

Fill trailer_opts with "unfold" and "only" to match the sub-arguments
given to the "%(trailers)" atom. Then, let's use the filled trailer_opts
instance with 'format_trailers_from_commit' in order to format trailers
in the desired manner.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  5 -
 ref-filter.c   | 32 +-
 t/t6300-for-each-ref.sh| 40 ++
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 1279b9733..4a2c851e6 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -218,7 +218,10 @@ blank line.  The optional GPG signature is 
`contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
 are obtained as `trailers` (or by using the historical alias
-`contents:trailers`).
+`contents:trailers`).  Non-trailer lines from the trailer block can be omitted
+with `trailers:only`. Whitespace-continuations can be removed from trailers so
+that each trailer appears on a line by itself with its full content with
+`trailers:unfold`. Both can be used together as `trailers:unfold,only`.
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index bc591f4f3..43ed10a5e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -82,6 +82,7 @@ static struct used_atom {
} remote_ref;
struct {
enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
C_SUB, C_TRAILERS } option;
+   struct process_trailer_options trailer_opts;
unsigned int nlines;
} contents;
struct {
@@ -182,9 +183,23 @@ static void subject_atom_parser(const struct ref_format 
*format, struct used_ato
 
 static void trailers_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
 {
-   if (arg)
-   die(_("%%(trailers) does not take arguments"));
+   struct string_list params = STRING_LIST_INIT_DUP;
+   int i;
+
+   if (arg) {
+   string_list_split(, arg, ',', -1);
+   for (i = 0; i < params.nr; i++) {
+   const char *s = params.items[i].string;
+   if (!strcmp(s, "unfold"))
+   atom->u.contents.trailer_opts.unfold = 1;
+   else if (!strcmp(s, "only"))
+   atom->u.contents.trailer_opts.only_trailers = 1;
+   else
+   die(_("unknown %%(trailers) argument: %s"), s);
+   }
+   }
atom->u.contents.option = C_TRAILERS;
+   string_list_clear(, 0);
 }
 
 static void contents_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
@@ -1042,7 +1057,7 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
name++;
if (strcmp(name, "subject") &&
strcmp(name, "body") &&
-   strcmp(name, "trailers") &&
+   !starts_with(name, "trailers") &&
!starts_with(name, "contents"))
continue;
if (!subpos)
@@ -1067,13 +1082,12 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
append_lines(, subpos, contents_end - subpos, 
atom->u.contents.nlines);
v->s = strbuf_detach(, NULL);
} else if (atom->u.contents.option == C_TRAILERS) {
-   struct trailer_info info;
+   struct strbuf s = STRBUF_INIT;
 
-   /* Search for trailer info */
-   trailer_info_get(, subpos);
-   v->s = xmemdupz(info.trailer_start,
-   info.trailer_end - info.trailer_start);
-   trailer_info_release();
+   /* Format the trailer info according to the 
trailer_opts given */
+   format_trailers_from_commit(, subpos, 
>u.contents.trailer_opts);
+
+   v->s = strbuf_detach(, NULL);
} else if (atom->u.contents.option == C_BARE)
v->s = xstrdup(subpos);
}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 39431908d..e4ce9c550 100755
--- a/t/t6300-for-each-

[PATCH v4 4/6] doc: use modern "`"-style code fencing

[Taylor Blau]

"'"- (single-quote) styled code fencing is no longer considered modern
within the "Documentation/" subtree.

In preparation for adding additional information to this section of
git-for-each-ref(1)'s documentation, update old-style code fencing to
use "`"-style fencing instead.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 323ce07de..1279b9733 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -213,12 +213,12 @@ and `date` to extract the named component.
 The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 of all lines of the commit message up to the first blank line.  The next
-line is 'contents:body', where body is all of the lines after the first
+line is `contents:body`, where body is all of the lines after the first
 blank line.  The optional GPG signature is `contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
-are obtained as 'trailers' (or by using the historical alias
-'contents:trailers').
+are obtained as `trailers` (or by using the historical alias
+`contents:trailers`).
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v4 2/6] t6300: refactor %(trailers) tests

[Taylor Blau]

We currently have one test for %(trailers) in `git-for-each-ref(1)`,
through "%(contents:trailers)". In preparation for more, let's add a few
things:

  - Move the commit creation step to its own test so that it can be
  re-used.

  - Add a non-trailer to the commit's trailers to test that non-trailers
  aren't shown using "%(trailers:only)".

  - Add a multi-line trailer to ensure that trailers are unfolded
  correctly using "%(trailers:unfold)".

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 t/t6300-for-each-ref.sh | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2274a4b73..39431908d 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -605,18 +605,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) 
on unborn branch' '
 cat >trailers <
 Signed-off-by: A U Thor <aut...@example.com>
+[ v2 updated patch description ]
+Acked-by: A U Thor
+  <aut...@example.com>
 EOF
 
-test_expect_success 'basic atom: head contents:trailers' '
+
+test_expect_success 'set up trailers for next test' '
echo "Some contents" > two &&
git add two &&
-   git commit -F - <<-EOF &&
+   git commit -F - <<-EOF
trailers: this commit message has trailers
 
Some message contents
 
$(cat trailers)
EOF
+'
+
+test_expect_success 'basic atom: head contents:trailers' '
git for-each-ref --format="%(contents:trailers)" refs/heads/master 
>actual &&
sanitize_pgp actual.clean &&
# git for-each-ref ends with a blank line
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH v4 0/6] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

Hi,

Attached is the fourth revision of my patch-set "Support %(trailers)
arguments in for-each-ref(1)".

It includes the following changes since v3:

  * Teach unfold() to unfold multiple lines.

  * Rebase patches to apply on top of master.

Thanks in advance, and thanks for all of your help thus far :-).


- Taylor

[PATCH v4 3/6] doc: 'trailers' is the preferred way to format trailers

[Taylor Blau]

The documentation makes reference to 'contents:trailers' as an example
to dig the trailers out of a commit. 'trailers' is an unmentioned
alternative, which is treated as an alias of 'contents:trailers'.

Since 'trailers' is easier to type, prefer that as the designated way to
dig out trailers information.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 66b4e0a40..323ce07de 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -217,7 +217,8 @@ line is 'contents:body', where body is all of the lines 
after the first
 blank line.  The optional GPG signature is `contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
-are obtained as 'contents:trailers'.
+are obtained as 'trailers' (or by using the historical alias
+'contents:trailers').
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v4 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

The %(contents) atom takes a contents "field" as its argument. Since
"trailers" is one of those fields, extend contents_atom_parser to parse
"trailers"'s arguments when used through "%(contents)", like:

  %(contents:trailers:unfold,only)

A caveat: trailers_atom_parser expects NULL when no arguments are given
(see: `parse_ref_filter_atom`). To simulate this behavior without
teaching trailers_atom_parser to accept strings with length zero,
conditionally pass NULL to trailers_atom_parser if the arguments portion
of the argument to %(contents) is empty.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 ref-filter.c|  6 --
 t/t6300-for-each-ref.sh | 37 +
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 43ed10a5e..2c03c2bf5 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -212,8 +212,10 @@ static void contents_atom_parser(const struct ref_format 
*format, struct used_at
atom->u.contents.option = C_SIG;
else if (!strcmp(arg, "subject"))
atom->u.contents.option = C_SUB;
-   else if (!strcmp(arg, "trailers"))
-   atom->u.contents.option = C_TRAILERS;
+   else if (skip_prefix(arg, "trailers", )) {
+   skip_prefix(arg, ":", );
+   trailers_atom_parser(atom, *arg ? NULL : arg);
+   }
else if (skip_prefix(arg, "lines=", )) {
atom->u.contents.option = C_LINES;
if (strtoul_ui(arg, 10, >u.contents.nlines))
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index e4ce9c550..ab95fe427 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -655,6 +655,35 @@ test_expect_success '%(trailers:only) and 
%(trailers:unfold) work together' '
test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
+   git for-each-ref --format="%(contents:trailers:unfold)" 
refs/heads/master >actual &&
+   {
+   unfold expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) shows only "key: value" 
trailers' '
+   git for-each-ref --format="%(contents:trailers:only)" refs/heads/master 
>actual &&
+   {
+   grep -v patch.description expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) 
work together' '
+   git for-each-ref --format="%(contents:trailers:only,unfold)" 
refs/heads/master >actual &&
+   git for-each-ref --format="%(contents:trailers:unfold,only)" 
refs/heads/master >reverse &&
+   test_cmp actual reverse &&
+   {
+   grep -v patch.description expect &&
+   test_cmp expect actual
+'
+
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
cat >expect <<-EOF &&
fatal: unknown %(trailers) argument: unsupported
@@ -663,6 +692,14 @@ test_expect_success '%(trailers) rejects unknown trailers 
arguments' '
   test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
+   cat >expect <<-EOF &&
+   fatal: unknown %(trailers) argument: unsupported
+   EOF
+  test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 
2>actual &&
+  test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
git for-each-ref --format="%(contents:trailers)" refs/heads/master 
>actual &&
sanitize_pgp actual.clean &&
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

Hi,

Attached is the sixth revision of my patch-set "Support %(trailers)
arguments in for-each-ref(1)".

In includes the following changes since v5:

  * Added an additional patch to change t4205 to harden `unfold()`
against multi-line trailer folding.

  * Added a missing parameter call in ref-filter.c to
`trailers_atom_parser` through `contents_atom_parser`.

I believe that this version of the series should be ready for queueing.

I am going to address Peff's follow-up for teaching
`parse_ref_atom_filter` to accept "empty" argument lists as
`%(refname:)` in a follow-up series later this evening.

Thanks again :-).


--
- Taylor

Re: [PATCH v4 4/6] doc: use modern "`"-style code fencing

[Taylor Blau]

On Mon, Oct 02, 2017 at 08:55:53AM +0900, Junio C Hamano wrote:
> Taylor Blau <m...@ttaylorr.com> writes:
>
> > "'"- (single-quote) styled code fencing is no longer considered modern
> > within the "Documentation/" subtree.
> >
> > In preparation for adding additional information to this section of
> > git-for-each-ref(1)'s documentation, update old-style code fencing to
> > use "`"-style fencing instead.
>
> Is this just me who wants to do s/fenc/quot/g?  Unless somebody
> objects, I'd do so while queuing.

I don't object, I think that fencing is less appropriate than quoting.
I couldn't find the term myself when writing this commit :-).

I am happy to send out v5 of this patch series with this commit
re-written, or you can change it while queuing. Whichever is easier for
you.

Thanks again.


--
- Taylor

Re: [PATCH v5 0/6] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

Hi,

Attached is the fifth revision of my patch-set "Support %(trailers)
arguments in for-each-ref(1)".

It includes the following changes since v4:

  * Clarified "ref-filter.c: parse trailers arguments with %(contents)
atom" to include reasoning for passing NULL as "" empty string in
contents_atom_parser.

  * Changed instances "fencing" to "quoting" in "doc: use modern
"`"-style code fencing".

  * Changed indentation in "pretty.c: delimit "%(trailers)" arguments
with ","" to silence the output from `make style`.

  * Fix incorrect indentation in "ref-filter.c: use trailer_opts to
format trailers".

Thanks again.


- Taylor

Re: [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

On Mon, Oct 02, 2017 at 01:05:07AM -0400, Jeff King wrote:
> On Sun, Oct 01, 2017 at 06:00:25PM +0900, Junio C Hamano wrote:
>
> > Taylor Blau <m...@ttaylorr.com> writes:
> >
> > > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> > > index 2a9fcf713..2bd0c5da7 100755
> > > --- a/t/t6300-for-each-ref.sh
> > > +++ b/t/t6300-for-each-ref.sh
> > > @@ -597,6 +597,9 @@ Acked-by: A U Thor
> > ><aut...@example.com>
> > >  EOF
> > >
> > > +unfold () {
> > > + perl -0pe 's/\n\s+/ /'
> > > +}
> >
> > For the purpose of the current shape of the test, the above might be
> > sufficient, but the lack of "/g" at the end means that the script
> > will happily stop after unfolding just one line, which probably is
> > not what you intended.
>
> This is indirectly my fault, since this was copied from my t4205
> version. It might be worth fixing while we're thinking about it, as it's
> a potential trap for future changes.

I agree. I'll include a fix for this as an additional commit in v6.

--
- Taylor

Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

On Mon, Oct 02, 2017 at 01:03:51AM -0400, Jeff King wrote:
> On Sun, Oct 01, 2017 at 05:33:04PM -0700, Taylor Blau wrote:
>
> > The %(contents) atom takes a contents "field" as its argument. Since
> > "trailers" is one of those fields, extend contents_atom_parser to parse
> > "trailers"'s arguments when used through "%(contents)", like:
> >
> >   %(contents:trailers:unfold,only)
> >
> > A caveat: trailers_atom_parser expects NULL when no arguments are given
> > (see: `parse_ref_filter_atom`). This is because string_list_split (given
> > a maxsplit of -1) returns a 1-ary string_list* containing the given
> > string if the delimiter could not be found using `strchr`.
> >
> > To simulate this behavior without teaching trailers_atom_parser to
> > accept strings with length zero, conditionally pass NULL to
> > trailers_atom_parser if the arguments portion of the argument to
> > %(contents) is empty.
>
> Doh, that string_list behavior is what I was missing in my earlier
> comments. I agree this is probably the best way of doing it. I'm tempted
> to say that parse_ref_filter_atom() should do a similar thing. Right now
> we've got:
>
>   $ git for-each-ref --format='%(refname)' | wc
>  22062206   79929
>   $ git for-each-ref --format='%(refname:short)' | wc
>  22062206   53622
>   $ git for-each-ref --format='%(refname:)' | wc
>   fatal: unrecognized %(refname:) argument:
>   0   0   0
>
> which seems a bit unfriendly. As we're on v6 here, I don't want to
> suggest it as part of this series. But maybe a #leftoverbits candidate,
> if others agree it's a good direction.

I think #leftoverbits is fine here, but I think addressing this before
2.15 is reasonable. I can take a look at this in a future patch series.

--
- Taylor

Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

On Mon, Oct 02, 2017 at 01:51:11PM +0900, Junio C Hamano wrote:
> Taylor Blau <m...@ttaylorr.com> writes:
>
> > @@ -212,9 +212,10 @@ static void contents_atom_parser(const struct 
> > ref_format *format, struct used_at
> > atom->u.contents.option = C_SIG;
> > else if (!strcmp(arg, "subject"))
> > atom->u.contents.option = C_SUB;
> > -   else if (!strcmp(arg, "trailers"))
> > -   atom->u.contents.option = C_TRAILERS;
> > -   else if (skip_prefix(arg, "lines=", )) {
> > +   else if (skip_prefix(arg, "trailers", )) {
> > +   skip_prefix(arg, ":", );
> > +   trailers_atom_parser(atom, *arg ? NULL : arg);
>
> A parameter for the call is missing.  I think you want 'format'
> there.

Thanks for pointing this out. I have fixed this in v6, which I am
sending out shortly. I think that this revision should be ready for
queueing.

--
- Taylor

[PATCH 0/1] ref-filter.c: pass empty-string as NULL to atom parsers

[Taylor Blau]

Hi,

Attached is a one-long patch series to un-distinguish between atoms
without sub-arguments ("%(refname)") and atoms with empty sub-argument
lists ("%(refname:)").

This addresses a user-experience issue that Peff points out:

> Doh, that string_list behavior is what I was missing in my earlier
> comments. I agree this is probably the best way of doing it. I'm tempted
> to say that parse_ref_filter_atom() should do a similar thing. Right now
> we've got:
>
>   $ git for-each-ref --format='%(refname)' | wc
>  22062206   79929
>   $ git for-each-ref --format='%(refname:short)' | wc
>  22062206   53622
>   $ git for-each-ref --format='%(refname:)' | wc
>   fatal: unrecognized %(refname:) argument:
>   0   0   0

By treating %(refname) and %(refname:) as the same thing. Peff has
convinced me that these _are_ indeed the same thing, as the first is a
%(refname) atom without any sub-arguments, and the later is a %(refname)
%atom with empty sub-arguments.

The reasoning is highlighted in the comment this patch adds, which makes
more ergonomic the use of string_list_split in atom parser
implementations.

Thank you in advance :-).


--
- Taylor

[PATCH v5 2/6] t6300: refactor %(trailers) tests

[Taylor Blau]

We currently have one test for %(trailers) in `git-for-each-ref(1)`,
through "%(contents:trailers)". In preparation for more, let's add a few
things:

  - Move the commit creation step to its own test so that it can be
  re-used.

  - Add a non-trailer to the commit's trailers to test that non-trailers
  aren't shown using "%(trailers:only)".

  - Add a multi-line trailer to ensure that trailers are unfolded
  correctly using "%(trailers:unfold)".

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 t/t6300-for-each-ref.sh | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2274a4b73..39431908d 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -605,18 +605,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) 
on unborn branch' '
 cat >trailers <
 Signed-off-by: A U Thor <aut...@example.com>
+[ v2 updated patch description ]
+Acked-by: A U Thor
+  <aut...@example.com>
 EOF
 
-test_expect_success 'basic atom: head contents:trailers' '
+
+test_expect_success 'set up trailers for next test' '
echo "Some contents" > two &&
git add two &&
-   git commit -F - <<-EOF &&
+   git commit -F - <<-EOF
trailers: this commit message has trailers
 
Some message contents
 
$(cat trailers)
EOF
+'
+
+test_expect_success 'basic atom: head contents:trailers' '
git for-each-ref --format="%(contents:trailers)" refs/heads/master 
>actual &&
sanitize_pgp actual.clean &&
# git for-each-ref ends with a blank line
-- 
2.14.1.145.gb3622a4ee

[PATCH v5 1/6] pretty.c: delimit "%(trailers)" arguments with ","

[Taylor Blau]

In preparation for adding consistent "%(trailers)" atom options to
`git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in
pretty.c to separate sub-arguments with a ",", instead of a ":".

Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
"%(trailers:only,unfold)".

This change disambiguates between "top-level" arguments, and arguments
given to the trailers atom itself. It is consistent with the behavior of
"%(upstream)" and "%(push)" atoms.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 pretty.c  | 32 +++-
 t/t4205-log-pretty-formats.sh |  4 ++--
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/pretty.c b/pretty.c
index 94eab5c89..7500fe694 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1056,6 +1056,23 @@ static size_t parse_padding_placeholder(struct strbuf 
*sb,
return 0;
 }
 
+static int match_placeholder_arg(const char *to_parse, const char *candidate,
+   const char **end)
+{
+   const char *p;
+   if (!(skip_prefix(to_parse, candidate, )))
+   return 0;
+   if (*p == ',') {
+   *end = p + 1;
+   return 1;
+   }
+   if (*p == ')') {
+   *end = p;
+   return 1;
+   }
+   return 0;
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
void *context)
@@ -1285,11 +1302,16 @@ static size_t format_commit_one(struct strbuf *sb, /* 
in UTF-8 */
 
if (skip_prefix(placeholder, "(trailers", )) {
struct process_trailer_options opts = 
PROCESS_TRAILER_OPTIONS_INIT;
-   while (*arg == ':') {
-   if (skip_prefix(arg, ":only", ))
-   opts.only_trailers = 1;
-   else if (skip_prefix(arg, ":unfold", ))
-   opts.unfold = 1;
+   if (*arg == ':') {
+   arg++;
+   for (;;) {
+   if (match_placeholder_arg(arg, "only", ))
+   opts.only_trailers = 1;
+   else if (match_placeholder_arg(arg, "unfold", 
))
+   opts.unfold = 1;
+   else
+   break;
+   }
}
if (*arg == ')') {
format_trailers_from_commit(sb, msg + c->subject_off, 
);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f53010..977472f53 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 '
 
 test_expect_success ':only and :unfold work together' '
-   git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
-   git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+   git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
+   git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
test_cmp actual reverse &&
{
grep -v patch.description 

[PATCH v5 3/6] doc: 'trailers' is the preferred way to format trailers

[Taylor Blau]

The documentation makes reference to 'contents:trailers' as an example
to dig the trailers out of a commit. 'trailers' is an unmentioned
alternative, which is treated as an alias of 'contents:trailers'.

Since 'trailers' is easier to type, prefer that as the designated way to
dig out trailers information.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 66b4e0a40..323ce07de 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -217,7 +217,8 @@ line is 'contents:body', where body is all of the lines 
after the first
 blank line.  The optional GPG signature is `contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
-are obtained as 'contents:trailers'.
+are obtained as 'trailers' (or by using the historical alias
+'contents:trailers').
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v5 5/6] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

Fill trailer_opts with "unfold" and "only" to match the sub-arguments
given to the "%(trailers)" atom. Then, let's use the filled trailer_opts
instance with 'format_trailers_from_commit' in order to format trailers
in the desired manner.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  5 -
 ref-filter.c   | 32 +-
 t/t6300-for-each-ref.sh| 40 ++
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 1279b9733..4a2c851e6 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -218,7 +218,10 @@ blank line.  The optional GPG signature is 
`contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
 are obtained as `trailers` (or by using the historical alias
-`contents:trailers`).
+`contents:trailers`).  Non-trailer lines from the trailer block can be omitted
+with `trailers:only`. Whitespace-continuations can be removed from trailers so
+that each trailer appears on a line by itself with its full content with
+`trailers:unfold`. Both can be used together as `trailers:unfold,only`.
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index bc591f4f3..43ed10a5e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -82,6 +82,7 @@ static struct used_atom {
} remote_ref;
struct {
enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
C_SUB, C_TRAILERS } option;
+   struct process_trailer_options trailer_opts;
unsigned int nlines;
} contents;
struct {
@@ -182,9 +183,23 @@ static void subject_atom_parser(const struct ref_format 
*format, struct used_ato
 
 static void trailers_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
 {
-   if (arg)
-   die(_("%%(trailers) does not take arguments"));
+   struct string_list params = STRING_LIST_INIT_DUP;
+   int i;
+
+   if (arg) {
+   string_list_split(, arg, ',', -1);
+   for (i = 0; i < params.nr; i++) {
+   const char *s = params.items[i].string;
+   if (!strcmp(s, "unfold"))
+   atom->u.contents.trailer_opts.unfold = 1;
+   else if (!strcmp(s, "only"))
+   atom->u.contents.trailer_opts.only_trailers = 1;
+   else
+   die(_("unknown %%(trailers) argument: %s"), s);
+   }
+   }
atom->u.contents.option = C_TRAILERS;
+   string_list_clear(, 0);
 }
 
 static void contents_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
@@ -1042,7 +1057,7 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
name++;
if (strcmp(name, "subject") &&
strcmp(name, "body") &&
-   strcmp(name, "trailers") &&
+   !starts_with(name, "trailers") &&
!starts_with(name, "contents"))
continue;
if (!subpos)
@@ -1067,13 +1082,12 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
append_lines(, subpos, contents_end - subpos, 
atom->u.contents.nlines);
v->s = strbuf_detach(, NULL);
} else if (atom->u.contents.option == C_TRAILERS) {
-   struct trailer_info info;
+   struct strbuf s = STRBUF_INIT;
 
-   /* Search for trailer info */
-   trailer_info_get(, subpos);
-   v->s = xmemdupz(info.trailer_start,
-   info.trailer_end - info.trailer_start);
-   trailer_info_release();
+   /* Format the trailer info according to the 
trailer_opts given */
+   format_trailers_from_commit(, subpos, 
>u.contents.trailer_opts);
+
+   v->s = strbuf_detach(, NULL);
} else if (atom->u.contents.option == C_BARE)
v->s = xstrdup(subpos);
}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 39431908d..54e52d4e9 100755
--- a/t/t6300-for-each-

[PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

The %(contents) atom takes a contents "field" as its argument. Since
"trailers" is one of those fields, extend contents_atom_parser to parse
"trailers"'s arguments when used through "%(contents)", like:

  %(contents:trailers:unfold,only)

A caveat: trailers_atom_parser expects NULL when no arguments are given
(see: `parse_ref_filter_atom`). This is because string_list_split (given
a maxsplit of -1) returns a 1-ary string_list* containing the given
string if the delimiter could not be found using `strchr`.

To simulate this behavior without teaching trailers_atom_parser to
accept strings with length zero, conditionally pass NULL to
trailers_atom_parser if the arguments portion of the argument to
%(contents) is empty.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 ref-filter.c|  7 ---
 t/t6300-for-each-ref.sh | 37 +
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 43ed10a5e..5b64de6ea 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format 
*format, struct used_at
atom->u.contents.option = C_SIG;
else if (!strcmp(arg, "subject"))
atom->u.contents.option = C_SUB;
-   else if (!strcmp(arg, "trailers"))
-   atom->u.contents.option = C_TRAILERS;
-   else if (skip_prefix(arg, "lines=", )) {
+   else if (skip_prefix(arg, "trailers", )) {
+   skip_prefix(arg, ":", );
+   trailers_atom_parser(atom, *arg ? NULL : arg);
+   } else if (skip_prefix(arg, "lines=", )) {
atom->u.contents.option = C_LINES;
if (strtoul_ui(arg, 10, >u.contents.nlines))
die(_("positive value expected contents:lines=%s"), 
arg);
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 54e52d4e9..872973b95 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -655,6 +655,35 @@ test_expect_success '%(trailers:only) and 
%(trailers:unfold) work together' '
test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
+   git for-each-ref --format="%(contents:trailers:unfold)" 
refs/heads/master >actual &&
+   {
+   unfold expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) shows only "key: value" 
trailers' '
+   git for-each-ref --format="%(contents:trailers:only)" refs/heads/master 
>actual &&
+   {
+   grep -v patch.description expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) 
work together' '
+   git for-each-ref --format="%(contents:trailers:only,unfold)" 
refs/heads/master >actual &&
+   git for-each-ref --format="%(contents:trailers:unfold,only)" 
refs/heads/master >reverse &&
+   test_cmp actual reverse &&
+   {
+   grep -v patch.description expect &&
+   test_cmp expect actual
+'
+
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
cat >expect <<-EOF &&
fatal: unknown %(trailers) argument: unsupported
@@ -663,6 +692,14 @@ test_expect_success '%(trailers) rejects unknown trailers 
arguments' '
test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
+   cat >expect <<-EOF &&
+   fatal: unknown %(trailers) argument: unsupported
+   EOF
+   test_must_fail git for-each-ref 
--format="%(contents:trailers:unsupported)" 2>actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
git for-each-ref --format="%(contents:trailers)" refs/heads/master 
>actual &&
sanitize_pgp actual.clean &&
-- 
2.14.1.145.gb3622a4ee

[PATCH v5 4/6] doc: use modern "`"-style code quoting

[Taylor Blau]

"'"- (single-quote) styled code quoting is no longer considered modern
within the "Documentation/" subtree.

In preparation for adding additional information to this section of
git-for-each-ref(1)'s documentation, update old-style code quoting to
use "`"-style quoting instead.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 323ce07de..1279b9733 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -213,12 +213,12 @@ and `date` to extract the named component.
 The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 of all lines of the commit message up to the first blank line.  The next
-line is 'contents:body', where body is all of the lines after the first
+line is `contents:body`, where body is all of the lines after the first
 blank line.  The optional GPG signature is `contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
-are obtained as 'trailers' (or by using the historical alias
-'contents:trailers').
+are obtained as `trailers` (or by using the historical alias
+`contents:trailers`).
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v6 6/7] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

Fill trailer_opts with "unfold" and "only" to match the sub-arguments
given to the "%(trailers)" atom. Then, let's use the filled trailer_opts
instance with 'format_trailers_from_commit' in order to format trailers
in the desired manner.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  5 -
 ref-filter.c   | 32 +-
 t/t6300-for-each-ref.sh| 40 ++
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 1279b9733..4a2c851e6 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -218,7 +218,10 @@ blank line.  The optional GPG signature is 
`contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
 are obtained as `trailers` (or by using the historical alias
-`contents:trailers`).
+`contents:trailers`).  Non-trailer lines from the trailer block can be omitted
+with `trailers:only`. Whitespace-continuations can be removed from trailers so
+that each trailer appears on a line by itself with its full content with
+`trailers:unfold`. Both can be used together as `trailers:unfold,only`.
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index bc591f4f3..43ed10a5e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -82,6 +82,7 @@ static struct used_atom {
} remote_ref;
struct {
enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
C_SUB, C_TRAILERS } option;
+   struct process_trailer_options trailer_opts;
unsigned int nlines;
} contents;
struct {
@@ -182,9 +183,23 @@ static void subject_atom_parser(const struct ref_format 
*format, struct used_ato
 
 static void trailers_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
 {
-   if (arg)
-   die(_("%%(trailers) does not take arguments"));
+   struct string_list params = STRING_LIST_INIT_DUP;
+   int i;
+
+   if (arg) {
+   string_list_split(, arg, ',', -1);
+   for (i = 0; i < params.nr; i++) {
+   const char *s = params.items[i].string;
+   if (!strcmp(s, "unfold"))
+   atom->u.contents.trailer_opts.unfold = 1;
+   else if (!strcmp(s, "only"))
+   atom->u.contents.trailer_opts.only_trailers = 1;
+   else
+   die(_("unknown %%(trailers) argument: %s"), s);
+   }
+   }
atom->u.contents.option = C_TRAILERS;
+   string_list_clear(, 0);
 }
 
 static void contents_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
@@ -1042,7 +1057,7 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
name++;
if (strcmp(name, "subject") &&
strcmp(name, "body") &&
-   strcmp(name, "trailers") &&
+   !starts_with(name, "trailers") &&
!starts_with(name, "contents"))
continue;
if (!subpos)
@@ -1067,13 +1082,12 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
append_lines(, subpos, contents_end - subpos, 
atom->u.contents.nlines);
v->s = strbuf_detach(, NULL);
} else if (atom->u.contents.option == C_TRAILERS) {
-   struct trailer_info info;
+   struct strbuf s = STRBUF_INIT;
 
-   /* Search for trailer info */
-   trailer_info_get(, subpos);
-   v->s = xmemdupz(info.trailer_start,
-   info.trailer_end - info.trailer_start);
-   trailer_info_release();
+   /* Format the trailer info according to the 
trailer_opts given */
+   format_trailers_from_commit(, subpos, 
>u.contents.trailer_opts);
+
+   v->s = strbuf_detach(, NULL);
} else if (atom->u.contents.option == C_BARE)
v->s = xstrdup(subpos);
}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 39431908d..54e52d4e9 100755
--- a/t/t6300-for-each-

[PATCH v6 5/7] t6300: refactor %(trailers) tests

[Taylor Blau]

We currently have one test for %(trailers) in `git-for-each-ref(1)`,
through "%(contents:trailers)". In preparation for more, let's add a few
things:

  - Move the commit creation step to its own test so that it can be
  re-used.

  - Add a non-trailer to the commit's trailers to test that non-trailers
  aren't shown using "%(trailers:only)".

  - Add a multi-line trailer to ensure that trailers are unfolded
  correctly using "%(trailers:unfold)".

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 t/t6300-for-each-ref.sh | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2274a4b73..39431908d 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -605,18 +605,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) 
on unborn branch' '
 cat >trailers <
 Signed-off-by: A U Thor <aut...@example.com>
+[ v2 updated patch description ]
+Acked-by: A U Thor
+  <aut...@example.com>
 EOF
 
-test_expect_success 'basic atom: head contents:trailers' '
+
+test_expect_success 'set up trailers for next test' '
echo "Some contents" > two &&
git add two &&
-   git commit -F - <<-EOF &&
+   git commit -F - <<-EOF
trailers: this commit message has trailers
 
Some message contents
 
$(cat trailers)
EOF
+'
+
+test_expect_success 'basic atom: head contents:trailers' '
git for-each-ref --format="%(contents:trailers)" refs/heads/master 
>actual &&
sanitize_pgp actual.clean &&
# git for-each-ref ends with a blank line
-- 
2.14.1.145.gb3622a4ee

[PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

The %(contents) atom takes a contents "field" as its argument. Since
"trailers" is one of those fields, extend contents_atom_parser to parse
"trailers"'s arguments when used through "%(contents)", like:

  %(contents:trailers:unfold,only)

A caveat: trailers_atom_parser expects NULL when no arguments are given
(see: `parse_ref_filter_atom`). This is because string_list_split (given
a maxsplit of -1) returns a 1-ary string_list* containing the given
string if the delimiter could not be found using `strchr`.

To simulate this behavior without teaching trailers_atom_parser to
accept strings with length zero, conditionally pass NULL to
trailers_atom_parser if the arguments portion of the argument to
%(contents) is empty.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 ref-filter.c|  7 ---
 t/t6300-for-each-ref.sh | 37 +
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 43ed10a5e..6c26b4733 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format 
*format, struct used_at
atom->u.contents.option = C_SIG;
else if (!strcmp(arg, "subject"))
atom->u.contents.option = C_SUB;
-   else if (!strcmp(arg, "trailers"))
-   atom->u.contents.option = C_TRAILERS;
-   else if (skip_prefix(arg, "lines=", )) {
+   else if (skip_prefix(arg, "trailers", )) {
+   skip_prefix(arg, ":", );
+   trailers_atom_parser(format, atom, *arg ? NULL : arg);
+   } else if (skip_prefix(arg, "lines=", )) {
atom->u.contents.option = C_LINES;
if (strtoul_ui(arg, 10, >u.contents.nlines))
die(_("positive value expected contents:lines=%s"), 
arg);
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 54e52d4e9..872973b95 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -655,6 +655,35 @@ test_expect_success '%(trailers:only) and 
%(trailers:unfold) work together' '
test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
+   git for-each-ref --format="%(contents:trailers:unfold)" 
refs/heads/master >actual &&
+   {
+   unfold expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) shows only "key: value" 
trailers' '
+   git for-each-ref --format="%(contents:trailers:only)" refs/heads/master 
>actual &&
+   {
+   grep -v patch.description expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) 
work together' '
+   git for-each-ref --format="%(contents:trailers:only,unfold)" 
refs/heads/master >actual &&
+   git for-each-ref --format="%(contents:trailers:unfold,only)" 
refs/heads/master >reverse &&
+   test_cmp actual reverse &&
+   {
+   grep -v patch.description expect &&
+   test_cmp expect actual
+'
+
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
cat >expect <<-EOF &&
fatal: unknown %(trailers) argument: unsupported
@@ -663,6 +692,14 @@ test_expect_success '%(trailers) rejects unknown trailers 
arguments' '
test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
+   cat >expect <<-EOF &&
+   fatal: unknown %(trailers) argument: unsupported
+   EOF
+   test_must_fail git for-each-ref 
--format="%(contents:trailers:unsupported)" 2>actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
git for-each-ref --format="%(contents:trailers)" refs/heads/master 
>actual &&
sanitize_pgp actual.clean &&
-- 
2.14.1.145.gb3622a4ee

[PATCH v6 4/7] doc: use modern "`"-style code quoting

[Taylor Blau]

"'"- (single-quote) styled code quoting is no longer considered modern
within the "Documentation/" subtree.

In preparation for adding additional information to this section of
git-for-each-ref(1)'s documentation, update old-style code quoting to
use "`"-style quoting instead.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 323ce07de..1279b9733 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -213,12 +213,12 @@ and `date` to extract the named component.
 The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 of all lines of the commit message up to the first blank line.  The next
-line is 'contents:body', where body is all of the lines after the first
+line is `contents:body`, where body is all of the lines after the first
 blank line.  The optional GPG signature is `contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
-are obtained as 'trailers' (or by using the historical alias
-'contents:trailers').
+are obtained as `trailers` (or by using the historical alias
+`contents:trailers`).
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v6 1/7] pretty.c: delimit "%(trailers)" arguments with ","

[Taylor Blau]

In preparation for adding consistent "%(trailers)" atom options to
`git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in
pretty.c to separate sub-arguments with a ",", instead of a ":".

Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
"%(trailers:only,unfold)".

This change disambiguates between "top-level" arguments, and arguments
given to the trailers atom itself. It is consistent with the behavior of
"%(upstream)" and "%(push)" atoms.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 pretty.c  | 32 +++-
 t/t4205-log-pretty-formats.sh |  4 ++--
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/pretty.c b/pretty.c
index 94eab5c89..7500fe694 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1056,6 +1056,23 @@ static size_t parse_padding_placeholder(struct strbuf 
*sb,
return 0;
 }
 
+static int match_placeholder_arg(const char *to_parse, const char *candidate,
+   const char **end)
+{
+   const char *p;
+   if (!(skip_prefix(to_parse, candidate, )))
+   return 0;
+   if (*p == ',') {
+   *end = p + 1;
+   return 1;
+   }
+   if (*p == ')') {
+   *end = p;
+   return 1;
+   }
+   return 0;
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
void *context)
@@ -1285,11 +1302,16 @@ static size_t format_commit_one(struct strbuf *sb, /* 
in UTF-8 */
 
if (skip_prefix(placeholder, "(trailers", )) {
struct process_trailer_options opts = 
PROCESS_TRAILER_OPTIONS_INIT;
-   while (*arg == ':') {
-   if (skip_prefix(arg, ":only", ))
-   opts.only_trailers = 1;
-   else if (skip_prefix(arg, ":unfold", ))
-   opts.unfold = 1;
+   if (*arg == ':') {
+   arg++;
+   for (;;) {
+   if (match_placeholder_arg(arg, "only", ))
+   opts.only_trailers = 1;
+   else if (match_placeholder_arg(arg, "unfold", 
))
+   opts.unfold = 1;
+   else
+   break;
+   }
}
if (*arg == ')') {
format_trailers_from_commit(sb, msg + c->subject_off, 
);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f53010..977472f53 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 '
 
 test_expect_success ':only and :unfold work together' '
-   git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
-   git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+   git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
+   git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
test_cmp actual reverse &&
{
grep -v patch.description 

[PATCH v6 3/7] doc: 'trailers' is the preferred way to format trailers

[Taylor Blau]

The documentation makes reference to 'contents:trailers' as an example
to dig the trailers out of a commit. 'trailers' is an unmentioned
alternative, which is treated as an alias of 'contents:trailers'.

Since 'trailers' is easier to type, prefer that as the designated way to
dig out trailers information.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 66b4e0a40..323ce07de 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -217,7 +217,8 @@ line is 'contents:body', where body is all of the lines 
after the first
 blank line.  The optional GPG signature is `contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
-are obtained as 'contents:trailers'.
+are obtained as 'trailers' (or by using the historical alias
+'contents:trailers').
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v6 2/7] t4205: unfold across multiple lines

[Taylor Blau]

Tests in t4205 test the following:

  git log --format='%(trailers:unfold)' ...

By ensuring the multi-line trailers are unfolded back onto the same
line. t4205 only includes tests for 2-line trailers, but `unfold()` will
fail for folded trailers on 3 or more lines.

In preparation for adding subsequent tests in t6300 that test similar
behavior in `git-for-each-ref(1)`, let's harden t4205 (and make it
consistent with the changes in t6300) by ensuring that 3 or more
line folded trailers are unfolded correctly.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 t/t4205-log-pretty-formats.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 977472f53..591f35daa 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -544,7 +544,7 @@ Signed-off-by: A U Thor
 EOF
 
 unfold () {
-   perl -0pe 's/\n\s+/ /'
+   perl -0pe 's/\n\s+/ /g'
 }
 
 test_expect_success 'set up trailer tests' '
-- 
2.14.1.145.gb3622a4ee

[PATCH] ref-filter.c: pass empty-string as NULL to atom parsers

[Taylor Blau]

Peff points out that different atom parsers handle the empty
"sub-argument" list differently. An example of this is the format
"%(refname:)".

Since callers often use `string_list_split` (which splits the empty
string with any delimiter as a 1-ary string_list containing the empty
string), this makes handling empty sub-argument strings non-ergonomic.

Let's fix this by assuming that atom parser implementations don't care
about distinguishing between the empty string "%(refname:)" and no
sub-arguments "%(refname)".

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 ref-filter.c| 17 -
 t/t6300-for-each-ref.sh |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index bc591f4f3..22be4097a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -415,8 +415,23 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
REALLOC_ARRAY(used_atom, used_atom_cnt);
used_atom[at].name = xmemdupz(atom, ep - atom);
used_atom[at].type = valid_atom[i].cmp_type;
-   if (arg)
+   if (arg) {
arg = used_atom[at].name + (arg - atom) + 1;
+   if (!*arg) {
+   /*
+* string_list_split is often use by atom parsers to
+* split multiple sub-arguments for inspection.
+*
+* Given a string that does not contain a delimiter
+* (example: ""), string_list_split returns a 1-ary
+* string_list that requires adding special cases to
+* atom parsers.
+*
+* Thus, treat the empty argument string as NULL.
+*/
+   arg = NULL;
+   }
+   }
memset(_atom[at].u, 0, sizeof(used_atom[at].u));
if (valid_atom[i].parser)
valid_atom[i].parser(format, _atom[at], arg);
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2274a4b73..edc1bd8ea 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -51,6 +51,7 @@ test_atom() {
 }
 
 test_atom head refname refs/heads/master
+test_atom head refname: refs/heads/master
 test_atom head refname:short master
 test_atom head refname:lstrip=1 heads/master
 test_atom head refname:lstrip=2 master
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

On Mon, Oct 02, 2017 at 02:51:00AM -0400, Jeff King wrote:
> > diff --git a/ref-filter.c b/ref-filter.c
> > index 43ed10a5e..6c26b4733 100644
> > --- a/ref-filter.c
> > +++ b/ref-filter.c
> > @@ -212,9 +212,10 @@ static void contents_atom_parser(const struct 
> > ref_format *format, struct used_at
> > atom->u.contents.option = C_SIG;
> > else if (!strcmp(arg, "subject"))
> > atom->u.contents.option = C_SUB;
> > -   else if (!strcmp(arg, "trailers"))
> > -   atom->u.contents.option = C_TRAILERS;
> > -   else if (skip_prefix(arg, "lines=", )) {
> > +   else if (skip_prefix(arg, "trailers", )) {
> > +   skip_prefix(arg, ":", );
> > +   trailers_atom_parser(format, atom, *arg ? NULL : arg);
>
> I think your logic is flipped. You want "*arg ? arg : NULL";

Thank you for pointing this out. I should have run "make test" on this
patch set (or, as you suggested, `git rebase -x "make test" HEAD~7`)
before sending it out. I appreciate you catching my mistake, and I'll
make sure to run "make test" more diligently in the future :-).

It sounds like Junio picked this up while queueing.

--
- Taylor

Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

On Mon, Oct 02, 2017 at 09:15:00PM +0900, Junio C Hamano wrote:
> Junio C Hamano  writes:
>
> > Thanks.  t6300 seems to show that %(contents:trailers:unfold) does
> > not quite work, among other things.
> >
> >   https://travis-ci.org/git/git/jobs/282126607#L3658
> >
> > I didn't have a chance to look into it myself.
>
> Peff's "oops, your logic is backwards" fixes the above failure.
>
> We also need this on top to pass the gettext-poison build.
>
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 872973b954..3bdfa02559 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -685,19 +685,21 @@ test_expect_success '%(contents:trailers:only) and 
> %(contents:trailers:unfold) w
>  '
>
>  test_expect_success '%(trailers) rejects unknown trailers arguments' '
> + # error message cannot be checked under i18n
>   cat >expect <<-EOF &&
>   fatal: unknown %(trailers) argument: unsupported
>   EOF
>   test_must_fail git for-each-ref --format="%(trailers:unsupported)" 
> 2>actual &&
> - test_cmp expect actual
> + test_i18ncmp expect actual
>  '
>
>  test_expect_success '%(contents:trailers) rejects unknown trailers 
> arguments' '
> + # error message cannot be checked under i18n
>   cat >expect <<-EOF &&
>   fatal: unknown %(trailers) argument: unsupported
>   EOF
>   test_must_fail git for-each-ref 
> --format="%(contents:trailers:unsupported)" 2>actual &&
> - test_cmp expect actual
> + test_i18ncmp expect actual
>  '
>
>  test_expect_success 'basic atom: head contents:trailers' '

Thank you for pointing this out. I am not well-versed on gettext, and
its usage within Git. I am happy to send out v7 of this series, or you
can apply these changes in queueing. Whichever is easier :-).

--
- Taylor

[PATCH v2] ref-filter.c: pass empty-string as NULL to atom parsers

[Taylor Blau]

Peff points out that different atom parsers handle the empty
"sub-argument" list differently. An example of this is the format
"%(refname:)".

Since callers often use `string_list_split` (which splits the empty
string with any delimiter as a 1-ary string_list containing the empty
string), this makes handling empty sub-argument strings non-ergonomic.

Let's fix this by assuming that atom parser implementations don't care
about distinguishing between the empty string "%(refname:)" and no
sub-arguments "%(refname)".

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 ref-filter.c| 10 +-
 t/t6300-for-each-ref.sh |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index bc591f4f3..f3e53d444 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -415,8 +415,16 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
REALLOC_ARRAY(used_atom, used_atom_cnt);
used_atom[at].name = xmemdupz(atom, ep - atom);
used_atom[at].type = valid_atom[i].cmp_type;
-   if (arg)
+   if (arg) {
arg = used_atom[at].name + (arg - atom) + 1;
+   if (!*arg) {
+   /*
+* Treat empty sub-arguments list as NULL (i.e.,
+* "%(atom:)" is equivalent to "%(atom)").
+*/
+   arg = NULL;
+   }
+   }
memset(_atom[at].u, 0, sizeof(used_atom[at].u));
if (valid_atom[i].parser)
valid_atom[i].parser(format, _atom[at], arg);
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2274a4b73..edc1bd8ea 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -51,6 +51,7 @@ test_atom() {
 }
 
 test_atom head refname refs/heads/master
+test_atom head refname: refs/heads/master
 test_atom head refname:short master
 test_atom head refname:lstrip=1 heads/master
 test_atom head refname:lstrip=2 master
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH] ref-filter.c: pass empty-string as NULL to atom parsers

[Taylor Blau]

On Mon, Oct 02, 2017 at 02:43:35AM -0400, Jeff King wrote:
> On Sun, Oct 01, 2017 at 10:53:11PM -0700, Taylor Blau wrote:
>
> > Peff points out that different atom parsers handle the empty
> > "sub-argument" list differently. An example of this is the format
> > "%(refname:)".
> >
> > Since callers often use `string_list_split` (which splits the empty
> > string with any delimiter as a 1-ary string_list containing the empty
> > string), this makes handling empty sub-argument strings non-ergonomic.
> >
> > Let's fix this by assuming that atom parser implementations don't care
> > about distinguishing between the empty string "%(refname:)" and no
> > sub-arguments "%(refname)".
>
> This looks good to me (both the explanation and the function of the
> code).

Thanks :-).

> But let me assume for a moment that your "please let me know" from the
> earlier series is still in effect, and you wish to be showered with
> pedantry and subjective advice. ;)
>
> I see a lot of newer contributors sending single patches as a 1-patch
> series with a cover letter. As a reviewer, I think this is mostly just a
> hassle. The cover letter ends up mostly repeating the same content from
> the single commit, so readers end up having to go over it twice (and you
> ended up having to write it twice).
>
> Sometimes there _is_ useful information to be conveyed that doesn't
> belong in the commit message, but that can easily go after the "---" (or
> before a "-- >8 --" if you really feel it should be read before the
> commit message.
>
> In general, if you find yourself writing a really long cover letter, and
> especially one that isn't mostly "meta" information (like where this
> should be applied, or what's changed since the last version), you should
> consider whether that information ought to go into the commit message
> instead. The one exception is if you _do_ have a long series and you
> need to sketch out the approach to help the reader see the big picture
> (in which case your cover letter should be summarizing what's already in
> the commit messages).

Thank you for this advice. I was worried when writing my cover letter
last night that it would be considered repetitive, but I wasn't sure how
much brevity/detail would be desired in a patch series of this length.

I'll keep this in mind for the future.

> And before anybody digs in the list to find my novel-length cover
> letters to throw back in my face, I know that I'm very guilty of this.
> I'm trying to get better at it, and passing it on so you can learn from
> my mistakes. :)

I appreciate your humility ;-).

> > -   if (arg)
> > +   if (arg) {
> > arg = used_atom[at].name + (arg - atom) + 1;
> > +   if (!*arg) {
> > +   /*
> > +* string_list_split is often use by atom parsers to
> > +* split multiple sub-arguments for inspection.
> > +*
> > +* Given a string that does not contain a delimiter
> > +* (example: ""), string_list_split returns a 1-ary
> > +* string_list that requires adding special cases to
> > +* atom parsers.
> > +*
> > +* Thus, treat the empty argument string as NULL.
> > +*/
> > +   arg = NULL;
> > +   }
> > +   }
>
> I know this is getting _really_ subjective, but IMHO this is a lot more
> reasoning than the comment needs. The commit message goes into the
> details of the "why", but here I'd have just written something like:
>
>   /* treat "%(foo:)" the same as "%(foo)"; i.e., no arguments */

I sent an updated v2 of this "series" (without a cover-letter) that
shortens this comment to more or less what you suggested. I've kept the
commit message longer, since I think that that information is useful
within "git blame".


--
- Taylor

Re: What's cooking in git.git (Oct 2017, #01; Wed, 4)

[Taylor Blau]

On Wed, Oct 04, 2017 at 05:46:21PM +0900, Junio C Hamano wrote:
> Jeff King  writes:
>
> >>  - pretty.c: delimit "%(trailers)" arguments with ","
> >>
> >>  "git for-each-ref --format=..." learned a new format element,
> >>  %(trailers), to show only the commit log trailer part of the log
> >>  message.
> >>
> >>  Will merge to 'next'.
> >
> > I think we want the first patch of this series to graduate before v2.15,
> > even if the rest doesn't make it. It tweaks a new syntax introduced
> > earlier in this cycle by jk/trailers-parse. If we ship without the
> > tweak, then we'll have to support the colon-delimiter to remain
> > backwards-compatible.
>
> Yeah, thanks for reminding me.  I actually was hoping that this will
> prove to be stable enough by the time -rc1 gets tagged, but yes, the
> bottom one looks innocuous/safe enough and should be fast-tracked to
> 'master' soonish.

It may make sense to send my other series to 'master' as well
("ref-filter.c: pass empty-string as NULL to atom parsers").

The series you're discussing here adds support for "empty" sub-agruments
(via: --format=%(contents:trailers:)), but Peff points out that this is
not a consistent user experience:

> Doh, that string_list behavior is what I was missing in my earlier
> comments. I agree this is probably the best way of doing it. I'm tempted
> to say that parse_ref_filter_atom() should do a similar thing. Right now
> we've got:
>
>   $ git for-each-ref --format='%(refname)' | wc
>  22062206   79929
>   $ git for-each-ref --format='%(refname:short)' | wc
>  22062206   53622
>   $ git for-each-ref --format='%(refname:)' | wc
>   fatal: unrecognized %(refname:) argument:
>   0   0   0

"ref-filter.c: pass empty-string as NULL to atom parsers" makes this
behavior of allowing empty sub-argument atom formats in
git-for-each-ref(1) consistently OK.

To avoid introducing a case where %(atom:) is sometimes allowed and
sometimes not, I would recommend that both of these patches be applied
to master at the same time.

--
- Taylor

Re: [PATCH] docs/pretty-formats: mention commas in %(trailers) syntax

[Taylor Blau]

On Fri, Dec 08, 2017 at 12:16:36AM -0500, Jeff King wrote:
> Commit 84ff053d47 (pretty.c: delimit "%(trailers)" arguments
> with ",", 2017-10-01) switched the syntax of the trailers
> placeholder, but forgot to update the documentation in
> pretty-formats.txt.
>
> There's need to mention the old syntax; it was never in a
> released version of Git.
>
> Signed-off-by: Jeff King 

My mistake, and thank you for giving this your attention.

[PATCH v4 4/7] grep.c: display column number of first match

[Taylor Blau]

To prepare for 'git grep' learning '--column', teach grep.c's
show_line() how to show the column of the first match on non-context
line.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 grep.c | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index fb0fa23231..f3fe416791 100644
--- a/grep.c
+++ b/grep.c
@@ -1364,7 +1364,7 @@ static int next_match(struct grep_opt *opt, char *bol, 
char *eol,
 }
 
 static void show_line(struct grep_opt *opt, char *bol, char *eol,
- const char *name, unsigned lno, char sign)
+ const char *name, unsigned lno, unsigned cno, char sign)
 {
int rest = eol - bol;
const char *match_color, *line_color = NULL;
@@ -1399,6 +1399,17 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
output_color(opt, buf, strlen(buf), opt->color_lineno);
output_sep(opt, sign);
}
+   /*
+* Treat 'cno' as the 1-indexed offset from the start of a non-context
+* line to its first match. Otherwise, 'cno' is 0 indicating that we are
+* being called with a context line.
+*/
+   if (opt->columnnum && cno) {
+   char buf[32];
+   xsnprintf(buf, sizeof(buf), "%d", cno);
+   output_color(opt, buf, strlen(buf), opt->color_columnno);
+   output_sep(opt, sign);
+   }
if (opt->color) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
@@ -1504,7 +1515,7 @@ static void show_funcname_line(struct grep_opt *opt, 
struct grep_source *gs,
break;
 
if (match_funcname(opt, gs, bol, eol)) {
-   show_line(opt, bol, eol, gs->name, lno, '=');
+   show_line(opt, bol, eol, gs->name, lno, 0, '=');
break;
}
}
@@ -1569,7 +1580,7 @@ static void show_pre_context(struct grep_opt *opt, struct 
grep_source *gs,
 
while (*eol != '\n')
eol++;
-   show_line(opt, bol, eol, gs->name, cur, sign);
+   show_line(opt, bol, eol, gs->name, cur, 0, sign);
bol = eol + 1;
cur++;
}
@@ -1833,7 +1844,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
show_pre_context(opt, gs, bol, eol, lno);
else if (opt->funcname)
show_funcname_line(opt, gs, bol, lno);
-   show_line(opt, bol, eol, gs->name, lno, ':');
+   show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, 
':');
last_hit = lno;
if (opt->funcbody)
show_function = 1;
@@ -1862,7 +1873,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
/* If the last hit is within the post context,
 * we need to show this line.
 */
-   show_line(opt, bol, eol, gs->name, lno, '-');
+   show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, 
'-');
}
 
next_line:
-- 
2.17.0

[PATCH v4 1/7] Documentation/config.txt: camel-case lineNumber for consistency

[Taylor Blau]

lineNumber has casing that is inconsistent with surrounding options,
like color.grep.matchContext, and color.grep.matchSelected. Re-case this
documentation in order to be consistent with the text around it, and to
ensure that new entries are consistent, too.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..6e8d969f52 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1157,7 +1157,7 @@ color.grep.::
filename prefix (when not using `-h`)
 `function`;;
function name lines (when using `-p`)
-`linenumber`;;
+`lineNumber`;;
line number prefix (when using `-n`)
 `match`;;
matching text (same as setting `matchContext` and `matchSelected`)
-- 
2.17.0

[PATCH v4 0/7] Teach '--column' to 'git-grep(1)'

[Taylor Blau]

Hi,

Attached is my fourth--and what I anticipate to be the final--re-roll of
my series to add teach 'git-grep(1)' a new '--column' flag.

Since last time, I have changed the following:

  * Respond to Ævar's review suggesting that I (1) change git-jump's
README, (2) use --column over --column-number, and (3) use /*\n, not
/**\n. [1].

This change comprises the majority of the inter-diff between v3..v4,
which is added below for conveniency. I have chosen to additionally
rename the configuration variables from columnNumber to column, to be
consistent with the new flag name.

Thanks in advance for your review. I am going to send out my next patch
(which Ævar suggested) to add '--only-matching' to 'git-grep(1)'.


Thanks,
Taylor

[1]: https://public-inbox.org/git/874lk2e4he@evledraar.gmail.com

Taylor Blau (7):
  Documentation/config.txt: camel-case lineNumber for consistency
  grep.c: expose matched column in match_line()
  grep.[ch]: extend grep_opt to allow showing matched column
  grep.c: display column number of first match
  builtin/grep.c: add '--column' option to 'git-grep(1)'
  grep.c: add configuration variables to show matched option
  contrib/git-jump/git-jump: jump to match column in addition to line

 Documentation/config.txt   |  7 ++-
 Documentation/git-grep.txt |  8 +++-
 builtin/grep.c |  1 +
 contrib/git-jump/README|  6 +++---
 contrib/git-jump/git-jump  |  2 +-
 grep.c | 39 +-
 grep.h |  2 ++
 t/t7810-grep.sh| 22 +
 8 files changed, 72 insertions(+), 15 deletions(-)

Inter-diff (since v3):

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8a2893d1e1..b3c861c5c3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1159,8 +1159,8 @@ color.grep.::
function name lines (when using `-p`)
 `lineNumber`;;
line number prefix (when using `-n`)
-`columnNumber`;;
-   column number prefix (when using `--column-number`)
+`column`;;
+   column number prefix (when using `--column`)
 `match`;;
matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1710,8 +1710,8 @@ gitweb.snapshot::
 grep.lineNumber::
If set to true, enable `-n` option by default.

-grep.columnNumber::
-   If set to true, enable the `--column-number` option by default.
+grep.column::
+   If set to true, enable the `--column` option by default.

 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index c5c4d712e6..d451cd8883 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -13,7 +13,7 @@ SYNOPSIS
   [-v | --invert-match] [-h|-H] [--full-name]
   [-E | --extended-regexp] [-G | --basic-regexp]
   [-P | --perl-regexp]
-  [-F | --fixed-strings] [-n | --line-number] [--column-number]
+  [-F | --fixed-strings] [-n | --line-number] [--column]
   [-l | --files-with-matches] [-L | --files-without-match]
   [(-O | --open-files-in-pager) []]
   [-z | --null]
@@ -44,8 +44,8 @@ CONFIGURATION
 grep.lineNumber::
If set to true, enable `-n` option by default.

-grep.columnNumber::
-   If set to true, enable the `--column-number` option by default.
+grep.column::
+   If set to true, enable the `--column` option by default.

 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
@@ -172,7 +172,7 @@ providing this option will cause it to die.
 --line-number::
Prefix the line number to matching lines.

---column-number::
+--column::
Prefix the 1-indexed column number of the first match on non-context 
lines.

 -l::
diff --git a/builtin/grep.c b/builtin/grep.c
index 512f60c591..5c83f17759 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -829,7 +829,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
GREP_PATTERN_TYPE_PCRE),
OPT_GROUP(""),
OPT_BOOL('n', "line-number", , N_("show line 
numbers")),
-   OPT_BOOL(0, "column-number", , N_("show column 
number of first match")),
+   OPT_BOOL(0, "column", , N_("show column number of 
first match")),
OPT_NEGBIT('h', NULL, , N_("don't show 
filenames"), 1),
OPT_BIT('H', NULL, , N_("show filenames"), 1),
OPT_NEGBIT(0, "full-name", ,
diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..7630e16854 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -35,7 +35,7 @@ Git-jump can generate four types of interesting lists:

   2. The beginning of any merge conflict markers.

-  3. Any

[PATCH v4 2/7] grep.c: expose matched column in match_line()

[Taylor Blau]

When calling match_line(), callers presently cannot determine the
relative offset of the match because match_line() discards the
'regmatch_t' that contains this information.

Instead, teach match_line() to take in a 'regmatch_t *' so that callers
can inspect the match's starting and ending offset from the beginning of
the line. This additional argument has no effect when opt->extended is
non-zero.

We will later pass the starting offset from 'regmatch_t *' to
show_line() in order to display the column number of the first match.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 grep.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 65b90c10a3..1c25782355 100644
--- a/grep.c
+++ b/grep.c
@@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, 
char *eol,
 }
 
 static int match_line(struct grep_opt *opt, char *bol, char *eol,
- enum grep_context ctx, int collect_hits)
+ regmatch_t *match, enum grep_context ctx,
+ int collect_hits)
 {
struct grep_pat *p;
-   regmatch_t match;
 
if (opt->extended)
return match_expr(opt, bol, eol, ctx, collect_hits);
 
/* we do not call with collect_hits without being extended */
for (p = opt->pattern_list; p; p = p->next) {
-   if (match_one_pattern(p, bol, eol, ctx, , 0))
+   if (match_one_pattern(p, bol, eol, ctx, match, 0))
return 1;
}
return 0;
@@ -1699,6 +1699,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
int try_lookahead = 0;
int show_function = 0;
struct userdiff_driver *textconv = NULL;
+   regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_HEAD;
xdemitconf_t xecfg;
 
@@ -1788,7 +1789,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
if ((ctx == GREP_CONTEXT_HEAD) && (eol == bol))
ctx = GREP_CONTEXT_BODY;
 
-   hit = match_line(opt, bol, eol, ctx, collect_hits);
+   hit = match_line(opt, bol, eol, , ctx, collect_hits);
*eol = ch;
 
if (collect_hits)
-- 
2.17.0

[PATCH v4 3/7] grep.[ch]: extend grep_opt to allow showing matched column

[Taylor Blau]

To support showing the matched column when calling 'git-grep(1)', teach
'grep_opt' the normal set of options to configure the default behavior
and colorization of this feature.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 grep.c | 3 +++
 grep.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/grep.c b/grep.c
index 1c25782355..fb0fa23231 100644
--- a/grep.c
+++ b/grep.c
@@ -46,6 +46,7 @@ void init_grep_defaults(void)
color_set(opt->color_filename, "");
color_set(opt->color_function, "");
color_set(opt->color_lineno, "");
+   color_set(opt->color_columnno, "");
color_set(opt->color_match_context, GIT_COLOR_BOLD_RED);
color_set(opt->color_match_selected, GIT_COLOR_BOLD_RED);
color_set(opt->color_selected, "");
@@ -155,6 +156,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
opt->extended_regexp_option = def->extended_regexp_option;
opt->pattern_type_option = def->pattern_type_option;
opt->linenum = def->linenum;
+   opt->columnnum = def->columnnum;
opt->max_depth = def->max_depth;
opt->pathname = def->pathname;
opt->relative = def->relative;
@@ -164,6 +166,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
color_set(opt->color_filename, def->color_filename);
color_set(opt->color_function, def->color_function);
color_set(opt->color_lineno, def->color_lineno);
+   color_set(opt->color_columnno, def->color_columnno);
color_set(opt->color_match_context, def->color_match_context);
color_set(opt->color_match_selected, def->color_match_selected);
color_set(opt->color_selected, def->color_selected);
diff --git a/grep.h b/grep.h
index 399381c908..08a0b391c5 100644
--- a/grep.h
+++ b/grep.h
@@ -127,6 +127,7 @@ struct grep_opt {
int prefix_length;
regex_t regexp;
int linenum;
+   int columnnum;
int invert;
int ignore_case;
int status_only;
@@ -159,6 +160,7 @@ struct grep_opt {
char color_filename[COLOR_MAXLEN];
char color_function[COLOR_MAXLEN];
char color_lineno[COLOR_MAXLEN];
+   char color_columnno[COLOR_MAXLEN];
char color_match_context[COLOR_MAXLEN];
char color_match_selected[COLOR_MAXLEN];
char color_selected[COLOR_MAXLEN];
-- 
2.17.0

[PATCH v4 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Taylor Blau]

Take advantage of 'git-grep(1)''s new option, '--column' in order to
teach Peff's 'git-jump' script how to jump to the correct column for any
given match.

'git-grep(1)''s output is in the correct format for Vim's jump list, so
no additional cleanup is necessary.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 contrib/git-jump/README   | 6 +++---
 contrib/git-jump/git-jump | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..7630e16854 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -35,7 +35,7 @@ Git-jump can generate four types of interesting lists:
 
   2. The beginning of any merge conflict markers.
 
-  3. Any grep matches.
+  3. Any grep matches, including the column of the first match on a line.
 
   4. Any whitespace errors detected by `git diff --check`.
 
@@ -65,7 +65,7 @@ git jump grep foo_bar
 git jump grep -i foo_bar
 
 # use the silver searcher for git jump grep
-git config jump.grepCmd "ag --column"
+git config jump.grepCmd "ag"
 --
 
 
@@ -82,7 +82,7 @@ which does something similar to `git jump grep`. However, it 
is limited
 to positioning the cursor to the correct line in only the first file,
 leaving you to locate subsequent hits in that file or other files using
 the editor or pager. By contrast, git-jump provides the editor with a
-complete list of files and line numbers for each match.
+complete list of files, lines, and a column number for each match.
 
 
 Limitations
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 80ab0590bc..931b0fe3a9 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
 # editor shows them to us in the status bar.
 mode_grep() {
cmd=$(git config jump.grepCmd)
-   test -n "$cmd" || cmd="git grep -n"
+   test -n "$cmd" || cmd="git grep -n --column"
$cmd "$@" |
perl -pe '
s/[ \t]+/ /g;
-- 
2.17.0

[PATCH v4 6/7] grep.c: add configuration variables to show matched option

[Taylor Blau]

To support git-grep(1)'s new option, '--column', document and teach
grep.c how to interpret relevant configuration options, similar to those
associated with '--line-number'.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/config.txt   | 5 +
 Documentation/git-grep.txt | 3 +++
 grep.c | 6 ++
 3 files changed, 14 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e8d969f52..b3c861c5c3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1159,6 +1159,8 @@ color.grep.::
function name lines (when using `-p`)
 `lineNumber`;;
line number prefix (when using `-n`)
+`column`;;
+   column number prefix (when using `--column`)
 `match`;;
matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1708,6 +1710,9 @@ gitweb.snapshot::
 grep.lineNumber::
If set to true, enable `-n` option by default.
 
+grep.column::
+   If set to true, enable the `--column` option by default.
+
 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
'fixed', or 'perl' will enable the `--basic-regexp`, 
`--extended-regexp`,
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 5409a24399..d451cd8883 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -44,6 +44,9 @@ CONFIGURATION
 grep.lineNumber::
If set to true, enable `-n` option by default.
 
+grep.column::
+   If set to true, enable the `--column` option by default.
+
 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
'fixed', or 'perl' will enable the `--basic-regexp`, 
`--extended-regexp`,
diff --git a/grep.c b/grep.c
index f3fe416791..37bb39a4a8 100644
--- a/grep.c
+++ b/grep.c
@@ -96,6 +96,10 @@ int grep_config(const char *var, const char *value, void *cb)
opt->linenum = git_config_bool(var, value);
return 0;
}
+   if (!strcmp(var, "grep.column")) {
+   opt->columnnum = git_config_bool(var, value);
+   return 0;
+   }
 
if (!strcmp(var, "grep.fullname")) {
opt->relative = !git_config_bool(var, value);
@@ -112,6 +116,8 @@ int grep_config(const char *var, const char *value, void 
*cb)
color = opt->color_function;
else if (!strcmp(var, "color.grep.linenumber"))
color = opt->color_lineno;
+   else if (!strcmp(var, "color.grep.column"))
+   color = opt->color_columnno;
else if (!strcmp(var, "color.grep.matchcontext"))
color = opt->color_match_context;
else if (!strcmp(var, "color.grep.matchselected"))
-- 
2.17.0

[PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

Teach 'git-grep(1)' a new option, '--column', to show the column
number of the first match on a non-context line.

For example:

  $ git grep -n --column foo | head -n3
  .clang-format:51:14:# myFunction(foo, bar, baz);
  .clang-format:64:7:# int foo();
  .clang-format:75:8:# void foo()

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-grep.txt |  5 -
 builtin/grep.c |  1 +
 t/t7810-grep.sh| 22 ++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 18b494731f..5409a24399 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -13,7 +13,7 @@ SYNOPSIS
   [-v | --invert-match] [-h|-H] [--full-name]
   [-E | --extended-regexp] [-G | --basic-regexp]
   [-P | --perl-regexp]
-  [-F | --fixed-strings] [-n | --line-number]
+  [-F | --fixed-strings] [-n | --line-number] [--column]
   [-l | --files-with-matches] [-L | --files-without-match]
   [(-O | --open-files-in-pager) []]
   [-z | --null]
@@ -169,6 +169,9 @@ providing this option will cause it to die.
 --line-number::
Prefix the line number to matching lines.
 
+--column::
+   Prefix the 1-indexed column number of the first match on non-context 
lines.
+
 -l::
 --files-with-matches::
 --name-only::
diff --git a/builtin/grep.c b/builtin/grep.c
index 5f32d2ce84..5c83f17759 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -829,6 +829,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
GREP_PATTERN_TYPE_PCRE),
OPT_GROUP(""),
OPT_BOOL('n', "line-number", , N_("show line 
numbers")),
+   OPT_BOOL(0, "column", , N_("show column number of 
first match")),
OPT_NEGBIT('h', NULL, , N_("don't show 
filenames"), 1),
OPT_BIT('H', NULL, , N_("show filenames"), 1),
OPT_NEGBIT(0, "full-name", ,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1797f632a3..a03c3416e7 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -99,6 +99,28 @@ do
test_cmp expected actual
'
 
+   test_expect_success "grep -w $L (with --column)" '
+   {
+   echo ${HC}file:5:foo mmap bar
+   echo ${HC}file:14:foo_mmap bar mmap
+   echo ${HC}file:5:foo mmap bar_mmap
+   echo ${HC}file:14:foo_mmap bar mmap baz
+   } >expected &&
+   git grep --column -w -e mmap $H >actual &&
+   test_cmp expected actual
+   '
+
+   test_expect_success "grep -w $L (with --{line,column}-number)" '
+   {
+   echo ${HC}file:1:5:foo mmap bar
+   echo ${HC}file:3:14:foo_mmap bar mmap
+   echo ${HC}file:4:5:foo mmap bar_mmap
+   echo ${HC}file:5:14:foo_mmap bar mmap baz
+   } >expected &&
+   git grep -n --column -w -e mmap $H >actual &&
+   test_cmp expected actual
+   '
+
test_expect_success "grep -w $L" '
{
echo ${HC}file:1:foo mmap bar
-- 
2.17.0

[PATCH 1/2] grep.c: extract show_line_header()

[Taylor Blau]

Teach 'git-grep(1)' how to print a line header multiple times from
show_line() in preparation for it learning '--only-matching'.

show_line_header() comprises of the code in show_line() executed in
order to produce a line header. It is a one-for-one extraction of the
existing implementation.

For now, show_line_header() provides no benefit over the change before
this patch. The following patch will call conditionally call
show_line_header() multiple times per invocation to show_line(), which
is the desired benefit of this change.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 grep.c | 44 +---
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/grep.c b/grep.c
index 37bb39a4a8..89dd719e4d 100644
--- a/grep.c
+++ b/grep.c
@@ -1369,26 +1369,9 @@ static int next_match(struct grep_opt *opt, char *bol, 
char *eol,
return hit;
 }
 
-static void show_line(struct grep_opt *opt, char *bol, char *eol,
- const char *name, unsigned lno, unsigned cno, char sign)
+static void show_line_header(struct grep_opt *opt, const char *name,
+ unsigned lno, unsigned cno, char sign)
 {
-   int rest = eol - bol;
-   const char *match_color, *line_color = NULL;
-
-   if (opt->file_break && opt->last_shown == 0) {
-   if (opt->show_hunk_mark)
-   opt->output(opt, "\n", 1);
-   } else if (opt->pre_context || opt->post_context || opt->funcbody) {
-   if (opt->last_shown == 0) {
-   if (opt->show_hunk_mark) {
-   output_color(opt, "--", 2, opt->color_sep);
-   opt->output(opt, "\n", 1);
-   }
-   } else if (lno > opt->last_shown + 1) {
-   output_color(opt, "--", 2, opt->color_sep);
-   opt->output(opt, "\n", 1);
-   }
-   }
if (opt->heading && opt->last_shown == 0) {
output_color(opt, name, strlen(name), opt->color_filename);
opt->output(opt, "\n", 1);
@@ -1416,6 +1399,29 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
output_color(opt, buf, strlen(buf), opt->color_columnno);
output_sep(opt, sign);
}
+}
+
+static void show_line(struct grep_opt *opt, char *bol, char *eol,
+ const char *name, unsigned lno, unsigned cno, char sign)
+{
+   int rest = eol - bol;
+   const char *match_color, *line_color = NULL;
+
+   if (opt->file_break && opt->last_shown == 0) {
+   if (opt->show_hunk_mark)
+   opt->output(opt, "\n", 1);
+   } else if (opt->pre_context || opt->post_context || opt->funcbody) {
+   if (opt->last_shown == 0) {
+   if (opt->show_hunk_mark) {
+   output_color(opt, "--", 2, opt->color_sep);
+   opt->output(opt, "\n", 1);
+   }
+   } else if (lno > opt->last_shown + 1) {
+   output_color(opt, "--", 2, opt->color_sep);
+   opt->output(opt, "\n", 1);
+   }
+   }
+   show_line_header(opt, name, lno, cno, sign);
if (opt->color) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
-- 
2.17.0

[PATCH 0/2] builtin/grep.c: teach '-o', '--only-matching'

[Taylor Blau]

Hi,

Attached is a series to teach 'git-grep(1)' how to respond to
'--only-matching' (a-la GNU grep(1)'s --only-matching, including an '-o'
synonym) to only print the matching component(s) of a line. It is based
on v4 of tb/grep-colno, which was sent in [1].

This was suggested to me by Ævar in [2].

This change was fairly straightforward, as Ævar suggests in [3], with
the only complication being that we must print a line header multiple
times when there are >1 matches per-line. This requirement pushes the
implementation towards the extraction of a show_line_header() function,
and some minor changes in show_line() to reflect.

Thank you in advance for your review.


Thanks,
Taylor

[1]: https://public-inbox.org/git/cover.1525488108.git...@ttaylorr.com
[2]: https://public-inbox.org/git/874lk2e4he@evledraar.gmail.com
[3]: https://public-inbox.org/git/87in9ucsbb@evledraar.gmail.com

Taylor Blau (2):
  grep.c: extract show_line_header()
  builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

 Documentation/git-grep.txt |  6 +++-
 builtin/grep.c |  1 +
 grep.c | 67 +-
 grep.h |  1 +
 t/t7810-grep.sh| 33 +++
 5 files changed, 85 insertions(+), 23 deletions(-)

--
2.17.0

[PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Taylor Blau]

Teach GNU grep(1)'s '-o' ('--only-matching') to 'git-grep'. This option
prints only the matching components of each line. It writes multiple
lines if more than one match exists on a given line.

For example:

  $ git grep -on --column --heading git -- README.md | head -3
  README.md
  15:56:git
  18:20:git

By using show_line_header(), 'git grep --only-matching' correctly
respects the '--header' option:

  $ git grep -on --column --heading git -- README.md | head -4
  README.md
  15:56:git
  18:20:git
  19:16:git

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-grep.txt |  6 +-
 builtin/grep.c |  1 +
 grep.c | 23 ---
 grep.h |  1 +
 t/t7810-grep.sh| 33 +
 5 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index d451cd8883..9754923041 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -20,7 +20,7 @@ SYNOPSIS
   [-c | --count] [--all-match] [-q | --quiet]
   [--max-depth ]
   [--color[=] | --no-color]
-  [--break] [--heading] [-p | --show-function]
+  [--break] [--heading] [-o | --only-matching] [-p | --show-function]
   [-A ] [-B ] [-C ]
   [-W | --function-context]
   [--threads ]
@@ -221,6 +221,10 @@ providing this option will cause it to die.
Show the filename above the matches in that file instead of
at the start of each shown line.
 
+--o::
+--only-matching::
+   Show only the matching part of the lines.
+
 -p::
 --show-function::
Show the preceding line that contains the function name of
diff --git a/builtin/grep.c b/builtin/grep.c
index 5c83f17759..5028bf96cf 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -851,6 +851,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
N_("print empty line between matches from different 
files")),
OPT_BOOL(0, "heading", ,
N_("show filename only once above matches from same 
file")),
+   OPT_BOOL('o', "only-matching", _matching, N_("show 
only matches")),
OPT_GROUP(""),
OPT_CALLBACK('C', "context", , N_("n"),
N_("show  context lines before and after matches"),
diff --git a/grep.c b/grep.c
index 89dd719e4d..da3f8e6266 100644
--- a/grep.c
+++ b/grep.c
@@ -1422,11 +1422,13 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
}
}
show_line_header(opt, name, lno, cno, sign);
-   if (opt->color) {
+   if (opt->color || opt->only_matching) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
int ch = *eol;
int eflags = 0;
+   int first = 1;
+   int offset = 1;
 
if (sign == ':')
match_color = opt->color_match_selected;
@@ -1443,16 +1445,31 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
if (match.rm_so == match.rm_eo)
break;
 
-   output_color(opt, bol, match.rm_so, line_color);
+   if (!opt->only_matching)
+   output_color(opt, bol, match.rm_so, line_color);
+   else if (!first) {
+   /*
+* We are given --only-matching, and this is not
+* the first match on a line. Reprint the
+* newline and header before showing another
+* match.
+*/
+   opt->output(opt, "\n", 1);
+   show_line_header(opt, name, lno,
+   offset+match.rm_so, sign);
+   }
output_color(opt, bol + match.rm_so,
 match.rm_eo - match.rm_so, match_color);
+   offset += match.rm_eo;
bol += match.rm_eo;
rest -= match.rm_eo;
eflags = REG_NOTBOL;
+   first = 0;
}
*eol = ch;
}
-   output_color(opt, bol, rest, line_color);
+   if (!opt->only_matching)
+   output_color(opt, bol, rest, line_color);
opt->output(opt, "\n", 1);
 }
 
diff --git a/grep.h b/grep.h
index 08a0b391c5..24c1460100 100644
--- a/grep.h
+++ b/grep.h
@@ -126,6 +126,7 @@ struct grep_opt {
const char *prefix;
int prefix_length;
regex

Re: [PATCH v4 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Taylor Blau]

On Sun, May 06, 2018 at 08:03:01PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Sun, May 06 2018, Martin Ågren wrote:
>
> > On 5 May 2018 at 04:43, Taylor Blau <m...@ttaylorr.com> wrote:
> >> Take advantage of 'git-grep(1)''s new option, '--column' in order to
> >> teach Peff's 'git-jump' script how to jump to the correct column for any
> >> given match.
> >>
> >> 'git-grep(1)''s output is in the correct format for Vim's jump list, so
> >> no additional cleanup is necessary.
> >
> >> diff --git a/contrib/git-jump/README b/contrib/git-jump/README
> >> index 4484bda410..7630e16854 100644
> >
> >>  # use the silver searcher for git jump grep
> >> -git config jump.grepCmd "ag --column"
> >> +git config jump.grepCmd "ag"
> >
> > I think this change originates from Ævar's comment that it "also makes
> > sense to update the example added in 007d06aa57 [...] which seems to
> > have added jump.grepCmd as a workaround for not having this" [1].
> >
> > Somehow though, this approach seems a bit backwards to me. I do believe
> > that your series reduces the reasons for using `jump.grepCmd`, but
> > regressing this example usage of `jump.grepCmd` seems a bit hostile. If
> > someone wants to use `ag`, wouldn't we want to hint that they will
> > probably want to use `--column`?
> >
> > Is there some other `ag --column --foo` that we can give, where `--foo`
> > is not yet in `git grep`? ;-)
> >
> > Martin
> >
> > [1] https://public-inbox.org/git/874lk2e4he@evledraar.gmail.com/
>
> Yeah it doesn't make sense to drop --column here, FWIW what I had in
> mind was something closer to:

Thanks; I wasn't quite clear on what you had suggested in [1], so the
attached diff is very helpful.

> diff --git a/contrib/git-jump/README b/contrib/git-jump/README
> index 4484bda410..357f79371a 100644
> --- a/contrib/git-jump/README
> +++ b/contrib/git-jump/README
> @@ -25,6 +25,13 @@ git-jump will feed this to the editor:
>  foo.c:2: printf("hello word!\n");
>  ---
>
> +Or, when running 'git jump grep' column numbers will also be emitted,
> +e.g. `git jump grep "hello"' would return:
> +
> +---
> +foo.c:2:10: printf("hello word!\n");
> +---
> +
>  Obviously this trivial case isn't that interesting; you could just open
>  `foo.c` yourself. But when you have many changes scattered across a
>  project, you can use the editor's support to "jump" from point to point.
>
> I.e. let's note what the output format is now like for 'grep', and no
> need to change the jump.grepCmd.

Applied (mostly) the above patch to my copy, and will attach as part of
v5.

> The above patch may be incorrect when it comes to the line numbe /
> column number / format, I just wrote that by hand.

Yes; the only thing that was wrong was the column number. The "w" is in
the 10th 1-indexed column, and 'git grep --column' uses 1-indexed
columns.

Thanks,
Taylor

Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

On Sat, May 05, 2018 at 08:15:03AM +0200, Duy Nguyen wrote:
> On Sat, May 5, 2018 at 4:43 AM, Taylor Blau <m...@ttaylorr.com> wrote:
> > Teach 'git-grep(1)' a new option, '--column', to show the column
> > number of the first match on a non-context line.
>
> Why? Or put it another way, what is this option used for? Only
> git-jump? (which should also be mentioned here if true)

Good question. My primary intention is giving
'contrib/git-jump/git-jump' the information it needs in order to tell
$EDITOR how to seek to the relevant position within a line.

I have amended this patch to include the relevant bits, and will attach
in v5.


Thanks,
Taylor

Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

On Sun, May 06, 2018 at 07:56:42PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Sat, May 05 2018, Taylor Blau wrote:
>
>
> > +   test_expect_success "grep -w $L (with --{line,column}-number)" '
>
> It's now --column in v4 but this still refers to v3 --column-number.

Thanks, I certainly missed this one.


Thanks,
Taylor

[PATCH v5 0/7] Teach '--column' to 'git-grep(1)'

[Taylor Blau]

Hi,

Attached is my fifth re-roll of the series to add '--column' to
'git-grep(1)'.

The main changes are from a René's concerns in [1]. He points out that
'--column' with certain extended expressions can produce nonsense
(particularly because we leave the regmatch_t uninitialized).

> So at least the combination of extended matches and --column should error
> out.  Supporting it would be better, of course.  That could get tricky for
> negations, though (e.g. git grep --not -e foo).

I have opted for the die() option, instead of supporting extended
matches with --column. The problem with extended matches in this case is
that _sometimes_ --column can produce sensible results, and other times
it cannot.

For instance, an extended expression containing a single atom
has a known answer for --column, but one containing a negative atom does
only sometimes. Specifically: if an NOT atom is at the top-level, we
_never_ have a sensible answer for --column, but only sometimes do when
it is not at the top-level.

So, this leaves us two choices: (1) don't support --column and extended
expressions, or (2) support --column with extended expressions by not
printing columnar information when we don't have an answer. Option (2)
requires callers to perform deep inspection of their extended
expressions, and determine whether or not there is a answer that Git
could produce. This is too much to ask a caller to reasonably consider
when scripting. On the other hand, option (1) does not allow the caller
to do as much under certain circumstances, but simplifies their lives
when scripting, etc. For those reasons, let's pick (1).

Beyond that, here is a summary of what has changed since last time:

  * die() when given --extended, or compiling to an extended grammar,
like in the case of 'git grep --column --not -e foo' [1].

  * Clarify patch 5/7 and indicate the intended purpose of supporting
'--column' [2].

  * Clarify that '--column' gives a 1-indexed _byte_ offset, nothing
else [3,5].

  * Remove a dangling reference to '--column-number' [4].

  * Clean up contrib/git-jump/README to Ævar's suggestion [6].


Thanks as always for your kind review.


Thanks,
Taylor

[1]: https://public-inbox.org/git/d030b4ee-5a92-4863-a29c-de2642bfa...@web.de
[2]: 
https://public-inbox.org/git/CACsJy8BdJ0=gwzqvfsqy-vjtzvt4uznzrapyxryxx2wnzal...@mail.gmail.com
[3]: 
https://public-inbox.org/git/20bc9baf-a85e-f00e-859e-e796ab432...@talktalk.net
[4]: https://public-inbox.org/git/87efioy8f9@evledraar.gmail.com
[5]: https://public-inbox.org/git/xmqqk1sfpn9j@gitster-ct.c.googlers.com
[6]: https://public-inbox.org/git/87d0y8y84q@evledraar.gmail.com/

Taylor Blau (7):
  Documentation/config.txt: camel-case lineNumber for consistency
  grep.c: expose matched column in match_line()
  grep.[ch]: extend grep_opt to allow showing matched column
  grep.c: display column number of first match
  builtin/grep.c: add '--column' option to 'git-grep(1)'
  grep.c: add configuration variables to show matched option
  contrib/git-jump/git-jump: jump to match column in addition to line

 Documentation/config.txt   |  7 ++-
 Documentation/git-grep.txt |  9 +++-
 builtin/grep.c |  4 
 contrib/git-jump/README| 12 +--
 contrib/git-jump/git-jump  |  2 +-
 grep.c | 42 ++
 grep.h |  2 ++
 t/t7810-grep.sh| 32 +
 8 files changed, 96 insertions(+), 14 deletions(-)

Inter-diff (since v5):

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index d451cd8883..dc8f76ce99 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -173,7 +173,8 @@ providing this option will cause it to die.
Prefix the line number to matching lines.

 --column::
-   Prefix the 1-indexed column number of the first match on non-context 
lines.
+   Prefix the 1-indexed byte-offset of the first match on non-context 
lines. This
+   option is incompatible with '--invert-match', and extended expressions.

 -l::
 --files-with-matches::
diff --git a/builtin/grep.c b/builtin/grep.c
index 5c83f17759..f9f516dfc4 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1112,6 +1112,9 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
hit = grep_objects(, , the_repository, );
}

+   if (opt.columnnum && opt.invert)
+   die(_("--column and --invert-match cannot be combined"));
+
if (num_threads)
hit |= wait_all();
if (hit && show_in_pager)
diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 7630e16854..2f618a7f97 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -25,6 +25,13 @@ git-jump will feed this to the editor:
 foo.c:2: printf("hello word!\n");
 ---

+Or, when running 'git jump grep', column 

[PATCH v5 6/7] grep.c: add configuration variables to show matched option

[Taylor Blau]

To support git-grep(1)'s new option, '--column', document and teach
grep.c how to interpret relevant configuration options, similar to those
associated with '--line-number'.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/config.txt   | 5 +
 Documentation/git-grep.txt | 3 +++
 grep.c | 6 ++
 3 files changed, 14 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e8d969f52..b3c861c5c3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1159,6 +1159,8 @@ color.grep.::
function name lines (when using `-p`)
 `lineNumber`;;
line number prefix (when using `-n`)
+`column`;;
+   column number prefix (when using `--column`)
 `match`;;
matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1708,6 +1710,9 @@ gitweb.snapshot::
 grep.lineNumber::
If set to true, enable `-n` option by default.
 
+grep.column::
+   If set to true, enable the `--column` option by default.
+
 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
'fixed', or 'perl' will enable the `--basic-regexp`, 
`--extended-regexp`,
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 75f1561112..dc8f76ce99 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -44,6 +44,9 @@ CONFIGURATION
 grep.lineNumber::
If set to true, enable `-n` option by default.
 
+grep.column::
+   If set to true, enable the `--column` option by default.
+
 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
'fixed', or 'perl' will enable the `--basic-regexp`, 
`--extended-regexp`,
diff --git a/grep.c b/grep.c
index f4228c23ac..5d904810ad 100644
--- a/grep.c
+++ b/grep.c
@@ -96,6 +96,10 @@ int grep_config(const char *var, const char *value, void *cb)
opt->linenum = git_config_bool(var, value);
return 0;
}
+   if (!strcmp(var, "grep.column")) {
+   opt->columnnum = git_config_bool(var, value);
+   return 0;
+   }
 
if (!strcmp(var, "grep.fullname")) {
opt->relative = !git_config_bool(var, value);
@@ -112,6 +116,8 @@ int grep_config(const char *var, const char *value, void 
*cb)
color = opt->color_function;
else if (!strcmp(var, "color.grep.linenumber"))
color = opt->color_lineno;
+   else if (!strcmp(var, "color.grep.column"))
+   color = opt->color_columnno;
else if (!strcmp(var, "color.grep.matchcontext"))
color = opt->color_match_context;
else if (!strcmp(var, "color.grep.matchselected"))
-- 
2.17.0

[PATCH v5 3/7] grep.[ch]: extend grep_opt to allow showing matched column

[Taylor Blau]

To support showing the matched column when calling 'git-grep(1)', teach
'grep_opt' the normal set of options to configure the default behavior
and colorization of this feature.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 grep.c | 3 +++
 grep.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/grep.c b/grep.c
index 1c25782355..fb0fa23231 100644
--- a/grep.c
+++ b/grep.c
@@ -46,6 +46,7 @@ void init_grep_defaults(void)
color_set(opt->color_filename, "");
color_set(opt->color_function, "");
color_set(opt->color_lineno, "");
+   color_set(opt->color_columnno, "");
color_set(opt->color_match_context, GIT_COLOR_BOLD_RED);
color_set(opt->color_match_selected, GIT_COLOR_BOLD_RED);
color_set(opt->color_selected, "");
@@ -155,6 +156,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
opt->extended_regexp_option = def->extended_regexp_option;
opt->pattern_type_option = def->pattern_type_option;
opt->linenum = def->linenum;
+   opt->columnnum = def->columnnum;
opt->max_depth = def->max_depth;
opt->pathname = def->pathname;
opt->relative = def->relative;
@@ -164,6 +166,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
color_set(opt->color_filename, def->color_filename);
color_set(opt->color_function, def->color_function);
color_set(opt->color_lineno, def->color_lineno);
+   color_set(opt->color_columnno, def->color_columnno);
color_set(opt->color_match_context, def->color_match_context);
color_set(opt->color_match_selected, def->color_match_selected);
color_set(opt->color_selected, def->color_selected);
diff --git a/grep.h b/grep.h
index 399381c908..08a0b391c5 100644
--- a/grep.h
+++ b/grep.h
@@ -127,6 +127,7 @@ struct grep_opt {
int prefix_length;
regex_t regexp;
int linenum;
+   int columnnum;
int invert;
int ignore_case;
int status_only;
@@ -159,6 +160,7 @@ struct grep_opt {
char color_filename[COLOR_MAXLEN];
char color_function[COLOR_MAXLEN];
char color_lineno[COLOR_MAXLEN];
+   char color_columnno[COLOR_MAXLEN];
char color_match_context[COLOR_MAXLEN];
char color_match_selected[COLOR_MAXLEN];
char color_selected[COLOR_MAXLEN];
-- 
2.17.0

[PATCH v5 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Taylor Blau]

Take advantage of 'git-grep(1)''s new option, '--column' in order to
teach Peff's 'git-jump' script how to jump to the correct column for any
given match.

'git-grep(1)''s output is in the correct format for Vim's jump list, so
no additional cleanup is necessary.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 contrib/git-jump/README   | 12 ++--
 contrib/git-jump/git-jump |  2 +-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..2f618a7f97 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -25,6 +25,13 @@ git-jump will feed this to the editor:
 foo.c:2: printf("hello word!\n");
 ---
 
+Or, when running 'git jump grep', column numbers will also be emitted,
+e.g. `git jump grep "hello"` would return:
+
+---
+foo.c:2:9: printf("hello word!\n");
+---
+
 Obviously this trivial case isn't that interesting; you could just open
 `foo.c` yourself. But when you have many changes scattered across a
 project, you can use the editor's support to "jump" from point to point.
@@ -35,7 +42,8 @@ Git-jump can generate four types of interesting lists:
 
   2. The beginning of any merge conflict markers.
 
-  3. Any grep matches.
+  3. Any grep matches, including the column of the first match on a
+ line.
 
   4. Any whitespace errors detected by `git diff --check`.
 
@@ -82,7 +90,7 @@ which does something similar to `git jump grep`. However, it 
is limited
 to positioning the cursor to the correct line in only the first file,
 leaving you to locate subsequent hits in that file or other files using
 the editor or pager. By contrast, git-jump provides the editor with a
-complete list of files and line numbers for each match.
+complete list of files, lines, and a column number for each match.
 
 
 Limitations
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 80ab0590bc..931b0fe3a9 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
 # editor shows them to us in the status bar.
 mode_grep() {
cmd=$(git config jump.grepCmd)
-   test -n "$cmd" || cmd="git grep -n"
+   test -n "$cmd" || cmd="git grep -n --column"
$cmd "$@" |
perl -pe '
s/[ \t]+/ /g;
-- 
2.17.0

[PATCH v5 4/7] grep.c: display column number of first match

[Taylor Blau]

To prepare for 'git grep' learning '--column', teach grep.c's
show_line() how to show the column of the first match on non-context
line.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 grep.c | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index fb0fa23231..f3fe416791 100644
--- a/grep.c
+++ b/grep.c
@@ -1364,7 +1364,7 @@ static int next_match(struct grep_opt *opt, char *bol, 
char *eol,
 }
 
 static void show_line(struct grep_opt *opt, char *bol, char *eol,
- const char *name, unsigned lno, char sign)
+ const char *name, unsigned lno, unsigned cno, char sign)
 {
int rest = eol - bol;
const char *match_color, *line_color = NULL;
@@ -1399,6 +1399,17 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
output_color(opt, buf, strlen(buf), opt->color_lineno);
output_sep(opt, sign);
}
+   /*
+* Treat 'cno' as the 1-indexed offset from the start of a non-context
+* line to its first match. Otherwise, 'cno' is 0 indicating that we are
+* being called with a context line.
+*/
+   if (opt->columnnum && cno) {
+   char buf[32];
+   xsnprintf(buf, sizeof(buf), "%d", cno);
+   output_color(opt, buf, strlen(buf), opt->color_columnno);
+   output_sep(opt, sign);
+   }
if (opt->color) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
@@ -1504,7 +1515,7 @@ static void show_funcname_line(struct grep_opt *opt, 
struct grep_source *gs,
break;
 
if (match_funcname(opt, gs, bol, eol)) {
-   show_line(opt, bol, eol, gs->name, lno, '=');
+   show_line(opt, bol, eol, gs->name, lno, 0, '=');
break;
}
}
@@ -1569,7 +1580,7 @@ static void show_pre_context(struct grep_opt *opt, struct 
grep_source *gs,
 
while (*eol != '\n')
eol++;
-   show_line(opt, bol, eol, gs->name, cur, sign);
+   show_line(opt, bol, eol, gs->name, cur, 0, sign);
bol = eol + 1;
cur++;
}
@@ -1833,7 +1844,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
show_pre_context(opt, gs, bol, eol, lno);
else if (opt->funcname)
show_funcname_line(opt, gs, bol, lno);
-   show_line(opt, bol, eol, gs->name, lno, ':');
+   show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, 
':');
last_hit = lno;
if (opt->funcbody)
show_function = 1;
@@ -1862,7 +1873,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
/* If the last hit is within the post context,
 * we need to show this line.
 */
-   show_line(opt, bol, eol, gs->name, lno, '-');
+   show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, 
'-');
}
 
next_line:
-- 
2.17.0

[PATCH v5 1/7] Documentation/config.txt: camel-case lineNumber for consistency

[Taylor Blau]

lineNumber has casing that is inconsistent with surrounding options,
like color.grep.matchContext, and color.grep.matchSelected. Re-case this
documentation in order to be consistent with the text around it, and to
ensure that new entries are consistent, too.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..6e8d969f52 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1157,7 +1157,7 @@ color.grep.::
filename prefix (when not using `-h`)
 `function`;;
function name lines (when using `-p`)
-`linenumber`;;
+`lineNumber`;;
line number prefix (when using `-n`)
 `match`;;
matching text (same as setting `matchContext` and `matchSelected`)
-- 
2.17.0

[PATCH v5 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

Teach 'git-grep(1)' a new option, '--column', to show the column
number of the first match on a non-context line. This makes it possible
to teach 'contrib/git-jump/git-jump' how to seek to the first matching
position of a grep match in your editor, and allows similar additional
scripting capabilities.

For example:

  $ git grep -n --column foo | head -n3
  .clang-format:51:14:# myFunction(foo, bar, baz);
  .clang-format:64:7:# int foo();
  .clang-format:75:8:# void foo()

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-grep.txt |  6 +-
 builtin/grep.c |  4 
 grep.c |  3 +++
 t/t7810-grep.sh| 32 
 4 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 18b494731f..75f1561112 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -13,7 +13,7 @@ SYNOPSIS
   [-v | --invert-match] [-h|-H] [--full-name]
   [-E | --extended-regexp] [-G | --basic-regexp]
   [-P | --perl-regexp]
-  [-F | --fixed-strings] [-n | --line-number]
+  [-F | --fixed-strings] [-n | --line-number] [--column]
   [-l | --files-with-matches] [-L | --files-without-match]
   [(-O | --open-files-in-pager) []]
   [-z | --null]
@@ -169,6 +169,10 @@ providing this option will cause it to die.
 --line-number::
Prefix the line number to matching lines.
 
+--column::
+   Prefix the 1-indexed byte-offset of the first match on non-context 
lines. This
+   option is incompatible with '--invert-match', and extended expressions.
+
 -l::
 --files-with-matches::
 --name-only::
diff --git a/builtin/grep.c b/builtin/grep.c
index 5f32d2ce84..f9f516dfc4 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -829,6 +829,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
GREP_PATTERN_TYPE_PCRE),
OPT_GROUP(""),
OPT_BOOL('n', "line-number", , N_("show line 
numbers")),
+   OPT_BOOL(0, "column", , N_("show column number of 
first match")),
OPT_NEGBIT('h', NULL, , N_("don't show 
filenames"), 1),
OPT_BIT('H', NULL, , N_("show filenames"), 1),
OPT_NEGBIT(0, "full-name", ,
@@ -,6 +1112,9 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
hit = grep_objects(, , the_repository, );
}
 
+   if (opt.columnnum && opt.invert)
+   die(_("--column and --invert-match cannot be combined"));
+
if (num_threads)
hit |= wait_all();
if (hit && show_in_pager)
diff --git a/grep.c b/grep.c
index f3fe416791..f4228c23ac 100644
--- a/grep.c
+++ b/grep.c
@@ -995,6 +995,9 @@ static void compile_grep_patterns_real(struct grep_opt *opt)
else if (!opt->extended && !opt->debug)
return;
 
+   if (opt->columnnum && opt->extended)
+   die(_("--column and extended expressions cannot be combined"));
+
p = opt->pattern_list;
if (p)
opt->pattern_expression = compile_pattern_expr();
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1797f632a3..aa56b21ed9 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -99,6 +99,28 @@ do
test_cmp expected actual
'
 
+   test_expect_success "grep -w $L (with --column)" '
+   {
+   echo ${HC}file:5:foo mmap bar
+   echo ${HC}file:14:foo_mmap bar mmap
+   echo ${HC}file:5:foo mmap bar_mmap
+   echo ${HC}file:14:foo_mmap bar mmap baz
+   } >expected &&
+   git grep --column -w -e mmap $H >actual &&
+   test_cmp expected actual
+   '
+
+   test_expect_success "grep -w $L (with --line-number, --column)" '
+   {
+   echo ${HC}file:1:5:foo mmap bar
+   echo ${HC}file:3:14:foo_mmap bar mmap
+   echo ${HC}file:4:5:foo mmap bar_mmap
+   echo ${HC}file:5:14:foo_mmap bar mmap baz
+   } >expected &&
+   git grep -n --column -w -e mmap $H >actual &&
+   test_cmp expected actual
+   '
+
test_expect_success "grep -w $L" '
{
echo ${HC}file:1:foo mmap bar
@@ -1590,4 +1612,14 @@ test_expect_success 'grep does not report i-t-a and 
assume unchanged with -L' '
test_cmp expected actual
 '
 
+test_expect_success 'grep does not allow --column, --invert-match' '
+   test_must_fail git grep --column --invert-match pat 2>err &&
+   test_i18

Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

On Mon, May 07, 2018 at 11:13:12PM +0900, Junio C Hamano wrote:
> Taylor Blau <m...@ttaylorr.com> writes:
>
> > diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> > index 18b494731f..5409a24399 100644
> > --- a/Documentation/git-grep.txt
> > +++ b/Documentation/git-grep.txt
> > @@ -13,7 +13,7 @@ SYNOPSIS
> >[-v | --invert-match] [-h|-H] [--full-name]
> >[-E | --extended-regexp] [-G | --basic-regexp]
> >[-P | --perl-regexp]
> > -  [-F | --fixed-strings] [-n | --line-number]
> > +  [-F | --fixed-strings] [-n | --line-number] [--column]
> >[-l | --files-with-matches] [-L | --files-without-match]
> >[(-O | --open-files-in-pager) []]
> >[-z | --null]
> > @@ -169,6 +169,9 @@ providing this option will cause it to die.
> >  --line-number::
> > Prefix the line number to matching lines.
> >
> > +--column::
> > +   Prefix the 1-indexed column number of the first match on non-context 
> > lines.
> > +
>
> Two questions.
>
>  - It is fine that the leftmost column is 1, but what does this
>number count?  The number of bytes on the same line before the
>first byte of the hit (plus 1)?  The display width of the initial
>non-matching part of the line (plus 1) on a fixed-width terminal?
>The number of "characters"?  Something else?

The count is the byte offset from the 1-index (which is the beginning of
the line, as you noted).

Incidentally, Peff and I chatted briefly offline about this, and agree
that it makes the most sense, since (1) Vim treats it correctly, and (2)
we can't be sure of options like display width, character count, etc.,
without knowing the character encoding.

Nonetheless, other folks in this thread seem to be curious about this
as well. I'll add it to the documentation for --column in
Documentation/git-grep.txt.

>  - Does --column combined with -v make any sense?  If not, shouldn't
>the command error out when both are given at the same time?

I hadn't thought of this. They do not work together, since 'git grep -v
--column' would require us to either (1) not output the column number,
or (2) output '0', or some other non-value.

I think that both (1) and (2) require callers to complicate their
scripts to understand either approach. As such, I think that we should
die() here, and add a test in t7810 to ensure that that's indeed what
happens.

Does this seem sensible to include in v5?


Thanks,
Taylor

Re: [PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Taylor Blau]

On Sat, May 05, 2018 at 03:36:12AM -0400, Eric Sunshine wrote:
> On Sat, May 5, 2018 at 12:03 AM, Taylor Blau <m...@ttaylorr.com> wrote:
> > Teach GNU grep(1)'s '-o' ('--only-matching') to 'git-grep'. This option
> > prints only the matching components of each line. It writes multiple
> > lines if more than one match exists on a given line.
> >
> > For example:
> >
> >   $ git grep -on --column --heading git -- README.md | head -3
> >   README.md
> >   15:56:git
> >   18:20:git
> >
> > By using show_line_header(), 'git grep --only-matching' correctly
> > respects the '--header' option:
>
> What is the '--header' option? I don't see it used in any example.

I think '--header' is a typo for '--heading', which is used in the
following example.

> >   $ git grep -on --column --heading git -- README.md | head -4
> >   README.md
> >   15:56:git
> >   18:20:git
> >   19:16:git
>
> How does this example differ from the earlier example (other than
> showing 4 lines of output rather than 3)?

Ack. I clipped from my terminal what I meant to be the seocnd
example, and pasted it in for both examples. They are meant to be as
follows:

  1. 'git grep' without heading, showing the full line prefix, and
  2. 'git grep' with heading, showing the file heading with '--heading'.

The later has '| head -n4' on the end to include 3+1 lines (3 matches, 1
heading) whereas the former has '| head -n3' to include 3 lines (3
matches, no heading).

I have updated my patch locally to reflect this.


Thanks,
Taylor

Re: [GSoC] A blog about 'git stash' project

[Taylor Blau]

On Fri, May 04, 2018 at 12:48:21AM +0300, Paul-Sebastian Ungureanu wrote:
> Hello everybody,
>
> The community bonding period started. It is well known that for a greater
> rate of success, it is recommended to send weekly reports regarding project
> status. But, what would be a good form for such a report? I, for one, think
> that starting a blog is one of the best options because all the content will
> be stored in one place. Without further ado, I would like you to present my
> blog [1].

Hi Paul, and welcome to Git! I am looking forward to reading your
patches and any additional writing posted on your blog.

> Any feedback is greatly appreciated! Thank you!

Do you have a RSS feed that I can consume in a feed reader?

> [1]
> https://ungps.github.io/
>
> Best regards,
> Paul Ungureanu

Thanks,
Taylor

Re: [PATCH 1/2] grep.c: extract show_line_header()

[Taylor Blau]

On Sat, May 05, 2018 at 03:30:51AM -0400, Eric Sunshine wrote:
> On Sat, May 5, 2018 at 12:03 AM, Taylor Blau <m...@ttaylorr.com> wrote:
> > Teach 'git-grep(1)' how to print a line header multiple times from
> > show_line() in preparation for it learning '--only-matching'.
> >
> > show_line_header() comprises of the code in show_line() executed in
>
> s/of//

Ack -- thanks for pointing that out :-).

> > order to produce a line header. It is a one-for-one extraction of the
> > existing implementation.
> >
> > For now, show_line_header() provides no benefit over the change before
> > this patch. The following patch will call conditionally call
>
> s/call conditionally call/conditionally call/

Double ack. Thanks again :-).


Thanks,
Taylor

[PATCH v5 2/7] grep.c: expose matched column in match_line()

[Taylor Blau]

When calling match_line(), callers presently cannot determine the
relative offset of the match because match_line() discards the
'regmatch_t' that contains this information.

Instead, teach match_line() to take in a 'regmatch_t *' so that callers
can inspect the match's starting and ending offset from the beginning of
the line. This additional argument has no effect when opt->extended is
non-zero.

We will later pass the starting offset from 'regmatch_t *' to
show_line() in order to display the column number of the first match.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 grep.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 65b90c10a3..1c25782355 100644
--- a/grep.c
+++ b/grep.c
@@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, 
char *eol,
 }
 
 static int match_line(struct grep_opt *opt, char *bol, char *eol,
- enum grep_context ctx, int collect_hits)
+ regmatch_t *match, enum grep_context ctx,
+ int collect_hits)
 {
struct grep_pat *p;
-   regmatch_t match;
 
if (opt->extended)
return match_expr(opt, bol, eol, ctx, collect_hits);
 
/* we do not call with collect_hits without being extended */
for (p = opt->pattern_list; p; p = p->next) {
-   if (match_one_pattern(p, bol, eol, ctx, , 0))
+   if (match_one_pattern(p, bol, eol, ctx, match, 0))
return 1;
}
return 0;
@@ -1699,6 +1699,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
int try_lookahead = 0;
int show_function = 0;
struct userdiff_driver *textconv = NULL;
+   regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_HEAD;
xdemitconf_t xecfg;
 
@@ -1788,7 +1789,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
if ((ctx == GREP_CONTEXT_HEAD) && (eol == bol))
ctx = GREP_CONTEXT_BODY;
 
-   hit = match_line(opt, bol, eol, ctx, collect_hits);
+   hit = match_line(opt, bol, eol, , ctx, collect_hits);
*eol = ch;
 
if (collect_hits)
-- 
2.17.0

Re: [PATCH v5 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

On Wed, May 09, 2018 at 07:26:57PM +0200, Martin Ågren wrote:
> On 9 May 2018 at 12:41, Phillip Wood <phillip.w...@talktalk.net> wrote:
> > On 09/05/18 03:13, Taylor Blau wrote:
> >>
> >>   +--column::
> >> +   Prefix the 1-indexed byte-offset of the first match on non-context
> >> lines. This
> >> +   option is incompatible with '--invert-match', and extended
> >> expressions.
> >> +
> >
> >
> > Sorry to be fussy, but while this is clearer I think to could be improved to
> > make it clear that it is the offset from the start of the matching line.
> > Also the mention of 'extended expressions' made me think of 'grep -E' but I
> > think (correct me if I'm wrong) you mean the boolean options '--and',
> > '--not' and '--or'. The man page only uses the word extended when talking
> > about extended regexes. I think something like
> >
> > Print the 1-indexed byte-offset of the first match from the start of the
> > matching line. This option is incompatible with '--invert-match', '--and',
> > '--not' and '--or'.
> >
> > would be clearer
>
> >> +   if (opt->columnnum && opt->extended)
> >> +   die(_("--column and extended expressions cannot be 
> >> combined"));
> >> +
>
> Just so it doesn't get missed: Phillip's comment (which I agree with)
> about "extended" would apply here as well. This would work fine, no?

This check we should retain and change the wording to mention '--and',
'--or', and '--not' specifically.

> One thing to notice is that dying for `--column --not` in combination
> with patch 7/7 makes git-jump unable to handle `--not` (and friends).
> That would be a regression? I suppose git-jump could use a special
> `--maybe-column` which would be "please use --column, unless I give you
> something that won't play well with it". Or --column could do that kind
> of falling back on its own. Or, git-jump could scan the arguments to
> decide whether to use `--column` or not. Hmm... Tricky. :-/

Agree that this is tricky. I don't think that --maybe-column is a
direction that we should take for the reasons I outlined in the cover
letter. Like I said, there are cases under an extended grammar where we
can and cannot display meaningful column offsets.

With regards to regressing 'git-jump', I feel as if 'git-jump --not' is
an uncommon-enough case that I would be comfortable with the tradeoff.
If a caller _is_ using '--not' in 'git-jump', they can reconfigure
'jump.grepCmd' to work around this issue.

Perhaps this is worth warning about in 'git-jump'? Peff, what do you
think?


Thanks,
Taylor

Re: [PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Taylor Blau]

On Tue, May 08, 2018 at 01:25:17PM -0400, Jeff King wrote:
> On Sat, May 05, 2018 at 08:49:43AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> > > +test_expect_success 'grep --only-matching --heading' '
> > > + git grep --only-matching --heading --line-number --column mmap file 
> > > >actual &&
> > > + test_cmp expected actual
> > > +'
> > > +
> > >  cat >expected < > >  hello.c
> > >  4:int main(int argc, const char **argv)
> >
> > We should test this a lot more, I think a good way to do that would be
> > to extend this series by first importing GNU grep's -o tests, see
> > http://git.savannah.gnu.org/cgit/grep.git/tree/tests/foad1 they are
> > license-compatible. Then change the grep_test() function to call git
> > grep instead.
>
> I'm trying to figure out what GNU grep's tests are actually checking
> that we don't have. I see:
>
>  - they check that "-i" returns the actual found string in its original
>case. This seems like a subset of finding a non-trivial regex. I.e.,
>"foo.*" should find "foobar". We probably should have a test like
>that.
>
>  - they test multiple hits on the same line, which seems like an
>important and easy-to-screw-up case; we do that, too.

Agree.

>  - they test everything other thing with and without "-i" because those
>are apparently separate code paths in GNU grep, but I don't think
>that applies to us.
>
>  - they test each case with "-b", but we don't have that (we do test
>with "--column", which is good)

Right, I think that we can ignore these groups.

>  - they test with "-n", which we do here (we don't test without, but
>that seems like an unlikely bug, knowing how it is implemented)

Fair, let's leave that as is.

>  - they test with -H, but that is already the default for git-grep

Good, we can ignore this one.

>  - they test with context (-C3) for us. It looks like GNU grep omits
>context lines with "-o", but we show a bunch of blank lines. This is
>I guess a bug (though it seems kind of an odd combination to specify
>in the first place)

I'm torn on what to do here. Currently, with -C3, I get a bunch of lines
like:

  -

Which I think is technically _right_, but I agree that it is certainly
an odd combination of things to give to 'git grep'. I think that we
could either:

  1. Continue outputting blank lines,
  2. Ignore '-C' with '-o', or
  3. die() when given this combination.

I think that (1) is the most "correct" (for some definition), so I'm
inclined to adopt that approach. I suppose that (2) is closer to what
GNU grep offers, and that is the point of this series, so perhaps it
makes sense to pick that instead.

I'll go with (2) for now, but I would appreciate others' thoughts before
sending a subsequent re-roll of this series.

> So I think it probably makes sense to just pick through the list I just
> wrote and write our own tests than to try to import GNU grep's specific
> tests (and there's a ton of other unrelated tests in that file that may
> or may not even run with git-grep).

I agree. Since some of these cases are already covered, and some don't
have analogues, I think that it is most sensible to go through the above
and add _those_, instead of porting the whole test suite over from GNU.

> > It should also be tested with the various grep.patternType options to
> > make sure it works with basic, extended, perl, fixed etc.
>
> Hmm, this code is all external to the actual matching. So unless one of
> those is totally screwing up the regmatch_t return, I'm not sure that's
> accomplishing much (and if one of them is, it's totally broken for
> coloring, "-c", and maybe other features).
>
> We've usually taken a pretty white-box approach to our testing, covering
> things that seem likely to go wrong given the general problem space and
> our implementation. But maybe I'm missing something in particular that
> you think might be tricky.
>
> -Peff

Thanks,
Taylor

Re: [PATCH v5 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

On Wed, May 09, 2018 at 06:17:20PM +0200, Duy Nguyen wrote:
> On Wed, May 9, 2018 at 4:13 AM, Taylor Blau <m...@ttaylorr.com> wrote:
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 5f32d2ce84..f9f516dfc4 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -829,6 +829,7 @@ int cmd_grep(int argc, const char **argv, const char 
> > *prefix)
> > GREP_PATTERN_TYPE_PCRE),
> > OPT_GROUP(""),
> > OPT_BOOL('n', "line-number", , N_("show line 
> > numbers")),
> > +   OPT_BOOL(0, "column", , N_("show column 
> > number of first match")),
>
> Two things to consider:
>
> - do we ever want columnar output in git-grep? Something like "git
> grep --column -l" could make sense (if you don't have very large
> worktree). --column is currently used for column output in git-branch,
> git-tag and git-status, which makes me think maybe we should reserve
> "--column" for that purpose and use another name here, even if we
> don't ever want column output in git-grep, for consistency.

I think that this is a valid concern. I had a similar thought when
adding 'git config --color' (as a new type specifier) that we might be
squatting on '--color' and instead opted for '[--type=]'.

I don't feel that the tradeoff between '--column' as a good name and the
concern that we _might_ want to output in a columnar format in 'git
grep' someday warrants the change.

> - If this is to help git-jump only and rarely manually specified on
> command line, you could add the flag PARSE_OPT_NOCOMPLETE to hide it
> from "git grep --" completion. You would need to use OPT_BOOL_F()
> instead of OPT_BOOL() in order to add extra flags.

I believe that this option is worth auto-completing. Its primarily
motivated for use within 'git-jump', but I feel as if it would be
useful for other callers, as well.

Thanks,
Taylor

Re: [PATCH v5 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

On Wed, May 09, 2018 at 11:41:02AM +0100, Phillip Wood wrote:
> Hi Taylor
>
> On 09/05/18 03:13, Taylor Blau wrote:
> > Teach 'git-grep(1)' a new option, '--column', to show the column
> > number of the first match on a non-context line. This makes it possible
> > to teach 'contrib/git-jump/git-jump' how to seek to the first matching
> > position of a grep match in your editor, and allows similar additional
> > scripting capabilities.
> >
> > For example:
> >
> >$ git grep -n --column foo | head -n3
> >.clang-format:51:14:# myFunction(foo, bar, baz);
> >.clang-format:64:7:# int foo();
> >.clang-format:75:8:# void foo()
> >
> > Signed-off-by: Taylor Blau <m...@ttaylorr.com>
> > ---
> >   Documentation/git-grep.txt |  6 +-
> >   builtin/grep.c |  4 
> >   grep.c |  3 +++
> >   t/t7810-grep.sh| 32 
> >   4 files changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> > index 18b494731f..75f1561112 100644
> > --- a/Documentation/git-grep.txt
> > +++ b/Documentation/git-grep.txt
> > @@ -13,7 +13,7 @@ SYNOPSIS
> >[-v | --invert-match] [-h|-H] [--full-name]
> >[-E | --extended-regexp] [-G | --basic-regexp]
> >[-P | --perl-regexp]
> > -  [-F | --fixed-strings] [-n | --line-number]
> > +  [-F | --fixed-strings] [-n | --line-number] [--column]
> >[-l | --files-with-matches] [-L | --files-without-match]
> >[(-O | --open-files-in-pager) []]
> >[-z | --null]
> > @@ -169,6 +169,10 @@ providing this option will cause it to die.
> >   --line-number::
> > Prefix the line number to matching lines.
> > +--column::
> > +   Prefix the 1-indexed byte-offset of the first match on non-context 
> > lines. This
> > +   option is incompatible with '--invert-match', and extended expressions.
> > +
>
> Sorry to be fussy, but while this is clearer I think to could be improved to
> make it clear that it is the offset from the start of the matching line.
> Also the mention of 'extended expressions' made me think of 'grep -E' but I
> think (correct me if I'm wrong) you mean the boolean options '--and',
> '--not' and '--or'. The man page only uses the word extended when talking
> about extended regexes. I think something like
>
> Print the 1-indexed byte-offset of the first match from the start of the
> matching line. This option is incompatible with '--invert-match', '--and',
> '--not' and '--or'.
>
> would be clearer

I agree, and would be happy to change it as-such. I think that there is
some pending discussion about regressing 'git-jump' no longer supporting
'--not', so I'll wait for that to resolve before resending this patch.

Thanks,
Taylor

[PATCH v2 1/2] grep.c: extract show_line_header()

[Taylor Blau]

Teach 'git-grep(1)' how to print a line header multiple times from
show_line() in preparation for it learning '--only-matching'.

show_line_header() comprises the code in show_line() executed in order
to produce a line header. It is a one-for-one extraction of the existing
implementation.

For now, show_line_header() provides no benefit over the change before
this patch. The following patch will conditionally call
show_line_header() multiple times per invocation to show_line(), which
is the desired benefit of this change.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 grep.c | 44 +---
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/grep.c b/grep.c
index 5ba1b05526..36bf7cf08d 100644
--- a/grep.c
+++ b/grep.c
@@ -1369,26 +1369,9 @@ static int next_match(struct grep_opt *opt, char *bol, 
char *eol,
return hit;
 }
 
-static void show_line(struct grep_opt *opt, char *bol, char *eol,
- const char *name, unsigned lno, unsigned cno, char sign)
+static void show_line_header(struct grep_opt *opt, const char *name,
+ unsigned lno, unsigned cno, char sign)
 {
-   int rest = eol - bol;
-   const char *match_color, *line_color = NULL;
-
-   if (opt->file_break && opt->last_shown == 0) {
-   if (opt->show_hunk_mark)
-   opt->output(opt, "\n", 1);
-   } else if (opt->pre_context || opt->post_context || opt->funcbody) {
-   if (opt->last_shown == 0) {
-   if (opt->show_hunk_mark) {
-   output_color(opt, "--", 2, opt->color_sep);
-   opt->output(opt, "\n", 1);
-   }
-   } else if (lno > opt->last_shown + 1) {
-   output_color(opt, "--", 2, opt->color_sep);
-   opt->output(opt, "\n", 1);
-   }
-   }
if (opt->heading && opt->last_shown == 0) {
output_color(opt, name, strlen(name), opt->color_filename);
opt->output(opt, "\n", 1);
@@ -1417,6 +1400,29 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
output_color(opt, buf, strlen(buf), opt->color_columnno);
output_sep(opt, sign);
}
+}
+
+static void show_line(struct grep_opt *opt, char *bol, char *eol,
+ const char *name, unsigned lno, unsigned cno, char sign)
+{
+   int rest = eol - bol;
+   const char *match_color, *line_color = NULL;
+
+   if (opt->file_break && opt->last_shown == 0) {
+   if (opt->show_hunk_mark)
+   opt->output(opt, "\n", 1);
+   } else if (opt->pre_context || opt->post_context || opt->funcbody) {
+   if (opt->last_shown == 0) {
+   if (opt->show_hunk_mark) {
+   output_color(opt, "--", 2, opt->color_sep);
+   opt->output(opt, "\n", 1);
+   }
+   } else if (lno > opt->last_shown + 1) {
+   output_color(opt, "--", 2, opt->color_sep);
+   opt->output(opt, "\n", 1);
+   }
+   }
+   show_line_header(opt, name, lno, cno, sign);
if (opt->color) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
-- 
2.17.0

[PATCH v2 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Taylor Blau]

Teach GNU grep(1)'s '-o' ('--only-matching') to 'git-grep'. This option
prints only the matching components of each line. It writes multiple
lines if more than one match exists on a given line.

For example:

  $ /git grep -on --column git -- README.md | head -3
  README.md:15:56:git
  README.md:18:20:git
  README.md:19:16:git

By using show_line_header(), 'git grep --only-matching' correctly
respects the '--heading' option:

  $ git grep -on --column --heading git -- README.md | head -4
  README.md
  15:56:git
  18:20:git
  19:16:git

We mirror GNU grep's behavior when given -A, -B, or -C with
--only-matching, by displaying only the matching portion(s) of a line,
ignoring contextual line(s), but displaying '--' (context separator)
line(s).

Notably: when show_line() is called on a line that contains _multiple_
matches, we keep track of a relative offset from the beginning of the
line and increment 'cno' in subsequent calls to show_line_header() when
the expression is not extended. In the extended case, we do not do this,
because the column of the first match is undefined, thus relative
offsets are meaningless.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-grep.txt |  6 +++-
 builtin/grep.c |  1 +
 grep.c | 34 +--
 grep.h |  1 +
 t/t7810-grep.sh| 69 ++
 5 files changed, 107 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index c48a578cb1..5c09abec4a 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -20,7 +20,7 @@ SYNOPSIS
   [-c | --count] [--all-match] [-q | --quiet]
   [--max-depth ]
   [--color[=] | --no-color]
-  [--break] [--heading] [-p | --show-function]
+  [--break] [--heading] [-o | --only-matching] [-p | --show-function]
   [-A ] [-B ] [-C ]
   [-W | --function-context]
   [--threads ]
@@ -223,6 +223,10 @@ providing this option will cause it to die.
Show the filename above the matches in that file instead of
at the start of each shown line.
 
+--o::
+--only-matching::
+   Prints only the matching part of the lines.
+
 -p::
 --show-function::
Show the preceding line that contains the function name of
diff --git a/builtin/grep.c b/builtin/grep.c
index f9f516dfc4..0507ac335a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -851,6 +851,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
N_("print empty line between matches from different 
files")),
OPT_BOOL(0, "heading", ,
N_("show filename only once above matches from same 
file")),
+   OPT_BOOL('o', "only-matching", _matching, N_("show 
only matches")),
OPT_GROUP(""),
OPT_CALLBACK('C', "context", , N_("n"),
N_("show  context lines before and after matches"),
diff --git a/grep.c b/grep.c
index 36bf7cf08d..9297fde643 100644
--- a/grep.c
+++ b/grep.c
@@ -1422,12 +1422,24 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
opt->output(opt, "\n", 1);
}
}
+   if (opt->only_matching && sign != ':') {
+   /*
+* If we're given '--only-matching' and the line is a contextual
+* one (i.e., we're given '-A', '-B', or '-C'), mark the line as
+* shown (to advance opt->last_shown), but do not show it (since
+* we are given '--only-matching').
+*/
+   opt->last_shown = lno;
+   return;
+   }
show_line_header(opt, name, lno, cno, sign);
-   if (opt->color) {
+   if (opt->color || opt->only_matching) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
int ch = *eol;
int eflags = 0;
+   int first = 1;
+   int offset = 1;
 
if (sign == ':')
match_color = opt->color_match_selected;
@@ -1444,16 +1456,32 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
if (match.rm_so == match.rm_eo)
break;
 
-   output_color(opt, bol, match.rm_so, line_color);
+   if (!opt->only_matching) {
+   output_color(opt, bol, match.rm_so, line_color);
+   } else if (!first) {
+   /*
+* We are given --only-matching, and this is not
+* the first match

[PATCH v2 0/2] builtin/grep.c: teach '-o', '--only-matching'

[Taylor Blau]

Hi,

Attached is the second re-roll of my series to add GNU grep's
'--only-matching' to git-grep.

The main thing that has changed since last time is our handling of
-{A,B,C}. Previously, as Peff points out in [1], we handle this in a
buggy way different than GNU.

I agree that although 'git grep -C -o ...' is an unusual invocation,
it is useful to (1) maintain as much consistency as reasonably makes
sense, and (2) to at least not be buggy.

I have also responded to Eric's suggestions in [2], and [3].

Thanks as always for your kind review :-).


Thanks,
Taylor

[1]: https://public-inbox.org/git/20180510064014.ga31...@sigill.intra.peff.net
[2]: 
https://public-inbox.org/git/capig+csrjww4-7vj6wk8aofnb20bqucsooysjdpci1r5vb8...@mail.gmail.com
[3]: 
https://public-inbox.org/git/capig+crbbz+qtqgiw_wq9e-groa-wtevp1vcrqmj5yqj8ty...@mail.gmail.com

Taylor Blau (2):
  grep.c: extract show_line_header()
  builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

 Documentation/git-grep.txt |  6 ++-
 builtin/grep.c |  1 +
 grep.c | 78 +++---
 grep.h |  1 +
 t/t7810-grep.sh| 69 +
 5 files changed, 132 insertions(+), 23 deletions(-)

--
2.17.0

[PATCH v6 4/7] grep.c: display column number of first match

[Taylor Blau]

To prepare for 'git grep' learning '--column', teach grep.c's
show_line() how to show the column of the first match on non-context
line.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 grep.c | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index fb0fa23231..f3fe416791 100644
--- a/grep.c
+++ b/grep.c
@@ -1364,7 +1364,7 @@ static int next_match(struct grep_opt *opt, char *bol, 
char *eol,
 }
 
 static void show_line(struct grep_opt *opt, char *bol, char *eol,
- const char *name, unsigned lno, char sign)
+ const char *name, unsigned lno, unsigned cno, char sign)
 {
int rest = eol - bol;
const char *match_color, *line_color = NULL;
@@ -1399,6 +1399,17 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
output_color(opt, buf, strlen(buf), opt->color_lineno);
output_sep(opt, sign);
}
+   /*
+* Treat 'cno' as the 1-indexed offset from the start of a non-context
+* line to its first match. Otherwise, 'cno' is 0 indicating that we are
+* being called with a context line.
+*/
+   if (opt->columnnum && cno) {
+   char buf[32];
+   xsnprintf(buf, sizeof(buf), "%d", cno);
+   output_color(opt, buf, strlen(buf), opt->color_columnno);
+   output_sep(opt, sign);
+   }
if (opt->color) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
@@ -1504,7 +1515,7 @@ static void show_funcname_line(struct grep_opt *opt, 
struct grep_source *gs,
break;
 
if (match_funcname(opt, gs, bol, eol)) {
-   show_line(opt, bol, eol, gs->name, lno, '=');
+   show_line(opt, bol, eol, gs->name, lno, 0, '=');
break;
}
}
@@ -1569,7 +1580,7 @@ static void show_pre_context(struct grep_opt *opt, struct 
grep_source *gs,
 
while (*eol != '\n')
eol++;
-   show_line(opt, bol, eol, gs->name, cur, sign);
+   show_line(opt, bol, eol, gs->name, cur, 0, sign);
bol = eol + 1;
cur++;
}
@@ -1833,7 +1844,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
show_pre_context(opt, gs, bol, eol, lno);
else if (opt->funcname)
show_funcname_line(opt, gs, bol, lno);
-   show_line(opt, bol, eol, gs->name, lno, ':');
+   show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, 
':');
last_hit = lno;
if (opt->funcbody)
show_function = 1;
@@ -1862,7 +1873,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
/* If the last hit is within the post context,
 * we need to show this line.
 */
-   show_line(opt, bol, eol, gs->name, lno, '-');
+   show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, 
'-');
}
 
next_line:
-- 
2.17.0

[PATCH v6 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

Teach 'git-grep(1)' a new option, '--column', to show the column
number of the first match on a non-context line. This makes it possible
to teach 'contrib/git-jump/git-jump' how to seek to the first matching
position of a grep match in your editor, and allows similar additional
scripting capabilities.

For example:

  $ git grep -n --column foo | head -n3
  .clang-format:51:14:# myFunction(foo, bar, baz);
  .clang-format:64:7:# int foo();
  .clang-format:75:8:# void foo()

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-grep.txt |  7 ++-
 builtin/grep.c |  4 
 grep.c |  5 +++--
 t/t7810-grep.sh| 39 ++
 4 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 18b494731f..cec4665df5 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -13,7 +13,7 @@ SYNOPSIS
   [-v | --invert-match] [-h|-H] [--full-name]
   [-E | --extended-regexp] [-G | --basic-regexp]
   [-P | --perl-regexp]
-  [-F | --fixed-strings] [-n | --line-number]
+  [-F | --fixed-strings] [-n | --line-number] [--column]
   [-l | --files-with-matches] [-L | --files-without-match]
   [(-O | --open-files-in-pager) []]
   [-z | --null]
@@ -169,6 +169,11 @@ providing this option will cause it to die.
 --line-number::
Prefix the line number to matching lines.
 
+--column::
+   Prefix the 1-indexed byte-offset of the first match from the start of 
the
+   matching line. This option is incompatible with '--invert-match', and
+   ignored with expressions using '--and', '--or', '--not'.
+
 -l::
 --files-with-matches::
 --name-only::
diff --git a/builtin/grep.c b/builtin/grep.c
index 5f32d2ce84..f9f516dfc4 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -829,6 +829,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
GREP_PATTERN_TYPE_PCRE),
OPT_GROUP(""),
OPT_BOOL('n', "line-number", , N_("show line 
numbers")),
+   OPT_BOOL(0, "column", , N_("show column number of 
first match")),
OPT_NEGBIT('h', NULL, , N_("don't show 
filenames"), 1),
OPT_BIT('H', NULL, , N_("show filenames"), 1),
OPT_NEGBIT(0, "full-name", ,
@@ -,6 +1112,9 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
hit = grep_objects(, , the_repository, );
}
 
+   if (opt.columnnum && opt.invert)
+   die(_("--column and --invert-match cannot be combined"));
+
if (num_threads)
hit |= wait_all();
if (hit && show_in_pager)
diff --git a/grep.c b/grep.c
index f3fe416791..7396b49a2d 100644
--- a/grep.c
+++ b/grep.c
@@ -1402,9 +1402,10 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
/*
 * Treat 'cno' as the 1-indexed offset from the start of a non-context
 * line to its first match. Otherwise, 'cno' is 0 indicating that we are
-* being called with a context line.
+* being called with a context line, or we are --extended, and cannot
+* always show an answer.
 */
-   if (opt->columnnum && cno) {
+   if (opt->columnnum && sign == ':' && !opt->extended) {
char buf[32];
xsnprintf(buf, sizeof(buf), "%d", cno);
output_color(opt, buf, strlen(buf), opt->color_columnno);
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1797f632a3..491b2e044a 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -99,6 +99,40 @@ do
test_cmp expected actual
'
 
+   test_expect_success "grep -w $L (with --column)" '
+   {
+   echo ${HC}file:5:foo mmap bar
+   echo ${HC}file:14:foo_mmap bar mmap
+   echo ${HC}file:5:foo mmap bar_mmap
+   echo ${HC}file:14:foo_mmap bar mmap baz
+   } >expected &&
+   git grep --column -w -e mmap $H >actual &&
+   test_cmp expected actual
+   '
+
+   test_expect_success "grep -w $L (with --column, -C)" '
+   {
+   echo ${HC}file:5:foo mmap bar
+   echo ${HC}file-foo_mmap bar
+   echo ${HC}file:14:foo_mmap bar mmap
+   echo ${HC}file:5:foo mmap bar_mmap
+   echo ${HC}file:14:foo_mmap bar mmap baz
+   } >expected &&
+   git grep --column -w -C1 -e mmap $H >actual &&
+   test_cmp expected actual
+   '
+
+   test_expect_success &q

[PATCH v6 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Taylor Blau]

Take advantage of 'git-grep(1)''s new option, '--column' in order to
teach Peff's 'git-jump' script how to jump to the correct column for any
given match.

'git-grep(1)''s output is in the correct format for Vim's jump list, so
no additional cleanup is necessary.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 contrib/git-jump/README   | 12 ++--
 contrib/git-jump/git-jump |  2 +-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..2f618a7f97 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -25,6 +25,13 @@ git-jump will feed this to the editor:
 foo.c:2: printf("hello word!\n");
 ---
 
+Or, when running 'git jump grep', column numbers will also be emitted,
+e.g. `git jump grep "hello"` would return:
+
+---
+foo.c:2:9: printf("hello word!\n");
+---
+
 Obviously this trivial case isn't that interesting; you could just open
 `foo.c` yourself. But when you have many changes scattered across a
 project, you can use the editor's support to "jump" from point to point.
@@ -35,7 +42,8 @@ Git-jump can generate four types of interesting lists:
 
   2. The beginning of any merge conflict markers.
 
-  3. Any grep matches.
+  3. Any grep matches, including the column of the first match on a
+ line.
 
   4. Any whitespace errors detected by `git diff --check`.
 
@@ -82,7 +90,7 @@ which does something similar to `git jump grep`. However, it 
is limited
 to positioning the cursor to the correct line in only the first file,
 leaving you to locate subsequent hits in that file or other files using
 the editor or pager. By contrast, git-jump provides the editor with a
-complete list of files and line numbers for each match.
+complete list of files, lines, and a column number for each match.
 
 
 Limitations
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 80ab0590bc..931b0fe3a9 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
 # editor shows them to us in the status bar.
 mode_grep() {
cmd=$(git config jump.grepCmd)
-   test -n "$cmd" || cmd="git grep -n"
+   test -n "$cmd" || cmd="git grep -n --column"
$cmd "$@" |
perl -pe '
s/[ \t]+/ /g;
-- 
2.17.0

[PATCH v6 6/7] grep.c: add configuration variables to show matched option

[Taylor Blau]

To support git-grep(1)'s new option, '--column', document and teach
grep.c how to interpret relevant configuration options, similar to those
associated with '--line-number'.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/config.txt   | 5 +
 Documentation/git-grep.txt | 3 +++
 grep.c | 6 ++
 3 files changed, 14 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e8d969f52..b3c861c5c3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1159,6 +1159,8 @@ color.grep.::
function name lines (when using `-p`)
 `lineNumber`;;
line number prefix (when using `-n`)
+`column`;;
+   column number prefix (when using `--column`)
 `match`;;
matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1708,6 +1710,9 @@ gitweb.snapshot::
 grep.lineNumber::
If set to true, enable `-n` option by default.
 
+grep.column::
+   If set to true, enable the `--column` option by default.
+
 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
'fixed', or 'perl' will enable the `--basic-regexp`, 
`--extended-regexp`,
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index cec4665df5..c48a578cb1 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -44,6 +44,9 @@ CONFIGURATION
 grep.lineNumber::
If set to true, enable `-n` option by default.
 
+grep.column::
+   If set to true, enable the `--column` option by default.
+
 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
'fixed', or 'perl' will enable the `--basic-regexp`, 
`--extended-regexp`,
diff --git a/grep.c b/grep.c
index 7396b49a2d..5ba1b05526 100644
--- a/grep.c
+++ b/grep.c
@@ -96,6 +96,10 @@ int grep_config(const char *var, const char *value, void *cb)
opt->linenum = git_config_bool(var, value);
return 0;
}
+   if (!strcmp(var, "grep.column")) {
+   opt->columnnum = git_config_bool(var, value);
+   return 0;
+   }
 
if (!strcmp(var, "grep.fullname")) {
opt->relative = !git_config_bool(var, value);
@@ -112,6 +116,8 @@ int grep_config(const char *var, const char *value, void 
*cb)
color = opt->color_function;
else if (!strcmp(var, "color.grep.linenumber"))
color = opt->color_lineno;
+   else if (!strcmp(var, "color.grep.column"))
+   color = opt->color_columnno;
else if (!strcmp(var, "color.grep.matchcontext"))
color = opt->color_match_context;
else if (!strcmp(var, "color.grep.matchselected"))
-- 
2.17.0

[PATCH v6 0/7] Teach '--column' to 'git-grep(1)'

[Taylor Blau]

Hi,

Attached is my sixth re-roll of a series to add '--column' to 'git
grep'.

The main change since v5 is supporting --column with queries containing
--and, --or, or --not. Previously, I had chosen to die() in this case
since there isn't always a good answer to "what is the first column of
?" but have gone back on this for two reasons:

  1. It is important not to regress calls to git-jump/contrib/git-jump
  that contain --and, --or, or --not.

  2. It is not that hard to detect the absence of column data in scripts.
 Likewise, git-jump will happily accept lines with or without
 columnar information, and Vim will accept it as-is.

So, let's support --column and only die() when also given
--invert-match. When we don't have a good answer, print nothing.

Thanks,
Taylor

Taylor Blau (7):
  Documentation/config.txt: camel-case lineNumber for consistency
  grep.c: expose matched column in match_line()
  grep.[ch]: extend grep_opt to allow showing matched column
  grep.c: display column number of first match
  builtin/grep.c: add '--column' option to 'git-grep(1)'
  grep.c: add configuration variables to show matched option
  contrib/git-jump/git-jump: jump to match column in addition to line

 Documentation/config.txt   |  7 ++-
 Documentation/git-grep.txt | 10 +-
 builtin/grep.c |  4 
 contrib/git-jump/README| 12 ++--
 contrib/git-jump/git-jump  |  2 +-
 grep.c | 40 +-
 grep.h |  2 ++
 t/t7810-grep.sh| 39 +
 8 files changed, 102 insertions(+), 14 deletions(-)

Inter-diff (since v5):

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index dc8f76ce99..c48a578cb1 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -173,8 +173,9 @@ providing this option will cause it to die.
Prefix the line number to matching lines.

 --column::
-   Prefix the 1-indexed byte-offset of the first match on non-context 
lines. This
-   option is incompatible with '--invert-match', and extended expressions.
+   Prefix the 1-indexed byte-offset of the first match from the start of 
the
+   matching line. This option is incompatible with '--invert-match', and
+   ignored with expressions using '--and', '--or', '--not'.

 -l::
 --files-with-matches::
diff --git a/grep.c b/grep.c
index 5d904810ad..5ba1b05526 100644
--- a/grep.c
+++ b/grep.c
@@ -1001,9 +1001,6 @@ static void compile_grep_patterns_real(struct grep_opt 
*opt)
else if (!opt->extended && !opt->debug)
return;

-   if (opt->columnnum && opt->extended)
-   die(_("--column and extended expressions cannot be combined"));
-
p = opt->pattern_list;
if (p)
opt->pattern_expression = compile_pattern_expr();
@@ -1411,9 +1408,10 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
/*
 * Treat 'cno' as the 1-indexed offset from the start of a non-context
 * line to its first match. Otherwise, 'cno' is 0 indicating that we are
-* being called with a context line.
+* being called with a context line, or we are --extended, and cannot
+* always show an answer.
 */
-   if (opt->columnnum && cno) {
+   if (opt->columnnum && sign == ':' && !opt->extended) {
char buf[32];
xsnprintf(buf, sizeof(buf), "%d", cno);
output_color(opt, buf, strlen(buf), opt->color_columnno);
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index aa56b21ed9..491b2e044a 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -110,6 +110,18 @@ do
test_cmp expected actual
'

+   test_expect_success "grep -w $L (with --column, -C)" '
+   {
+   echo ${HC}file:5:foo mmap bar
+   echo ${HC}file-foo_mmap bar
+   echo ${HC}file:14:foo_mmap bar mmap
+   echo ${HC}file:5:foo mmap bar_mmap
+   echo ${HC}file:14:foo_mmap bar mmap baz
+   } >expected &&
+   git grep --column -w -C1 -e mmap $H >actual &&
+   test_cmp expected actual
+   '
+
test_expect_success "grep -w $L (with --line-number, --column)" '
{
echo ${HC}file:1:5:foo mmap bar
@@ -1617,9 +1629,4 @@ test_expect_success 'grep does not allow --column, 
--invert-match' '
test_i18ngrep "\-\-column and \-\-invert-match cannot be combined" err
 '

-test_expect_success 'grep does not allow --column, extended' '
-   test_must_fail git grep --column --not -e pat 2>err &&
-   test_i18ngrep "\-\-column and extended expressions cannot be combined" 
err
-'
-
 test_done

--
2.17.0

[PATCH v6 2/7] grep.c: expose matched column in match_line()

[Taylor Blau]

When calling match_line(), callers presently cannot determine the
relative offset of the match because match_line() discards the
'regmatch_t' that contains this information.

Instead, teach match_line() to take in a 'regmatch_t *' so that callers
can inspect the match's starting and ending offset from the beginning of
the line. This additional argument has no effect when opt->extended is
non-zero.

We will later pass the starting offset from 'regmatch_t *' to
show_line() in order to display the column number of the first match.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 grep.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 65b90c10a3..1c25782355 100644
--- a/grep.c
+++ b/grep.c
@@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, 
char *eol,
 }
 
 static int match_line(struct grep_opt *opt, char *bol, char *eol,
- enum grep_context ctx, int collect_hits)
+ regmatch_t *match, enum grep_context ctx,
+ int collect_hits)
 {
struct grep_pat *p;
-   regmatch_t match;
 
if (opt->extended)
return match_expr(opt, bol, eol, ctx, collect_hits);
 
/* we do not call with collect_hits without being extended */
for (p = opt->pattern_list; p; p = p->next) {
-   if (match_one_pattern(p, bol, eol, ctx, , 0))
+   if (match_one_pattern(p, bol, eol, ctx, match, 0))
return 1;
}
return 0;
@@ -1699,6 +1699,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
int try_lookahead = 0;
int show_function = 0;
struct userdiff_driver *textconv = NULL;
+   regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_HEAD;
xdemitconf_t xecfg;
 
@@ -1788,7 +1789,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
if ((ctx == GREP_CONTEXT_HEAD) && (eol == bol))
ctx = GREP_CONTEXT_BODY;
 
-   hit = match_line(opt, bol, eol, ctx, collect_hits);
+   hit = match_line(opt, bol, eol, , ctx, collect_hits);
*eol = ch;
 
if (collect_hits)
-- 
2.17.0