* Re: 2.11.0-rc1 will not be tagged for a few days
From: Junio C Hamano @ 2016-11-09 6:29 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, git, Johannes Schindelin, Lars Schneider
In-Reply-To: <20161108214825.yo37kvoqkeucuqgg@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I'm collecting v2.11-rc1 topics in the "refs/heads/for-junio/" section
> of git://github.com/peff/git.git.
>
> I've also got proposed merges for "master" there, though note that none
> of the topics has actually cooked at all in next (the fixes are trivial
> enough that it may be OK, though).
I am not quite back up at full steam yet, but I expect I'd be more
or less functioning tomorrow, so I'll fetch them from your tree and
continue.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 01/16] Git.pm: add subroutines for commenting lines
From: Junio C Hamano @ 2016-11-09 1:06 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar
In-Reply-To: <20161108120823.11204-2-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> Add subroutines prefix_lines and comment_lines.
>
> Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
> ---
> perl/Git.pm | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index b2732822a..17be59fb7 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -1438,6 +1438,29 @@ sub END {
>
> } # %TEMP_* Lexical Context
>
> +=item prefix_lines ( PREFIX, STRING )
> +
> +Prefixes lines in C<STRING> with C<PREFIX>.
> +
> +=cut
> +
> +sub prefix_lines {
> + my ($prefix, $string) = @_;
> + $string =~ s/^/$prefix/mg;
> + return $string;
> +}
> +
> +=item comment_lines ( STRING )
> +
> +Comments lines following core.commentchar configuration.
> +
> +=cut
> +
> +sub comment_lines {
> + my $comment_line_char = config("core.commentchar") || '#';
> + return prefix_lines("$comment_line_char ", @_);
> +}
> +
This makes it appear as if comment_lines can take arbitrary number
of strings as its arguments (because the outer caller just passes @_
thru), but in fact because prefix_lines ignores anything other than
$_[0] and $_[1], only the first parameter given to comment_lineS sub
is inspected for lines in it and the prefix-char prefixed at the
beginning of each of them.
Which is not a great interface, as it is quite misleading.
Perhaps
prefix_lines("#", join("\n", @_));
or something like that may make it less confusing.
^ permalink raw reply
* Re: git submodule add broken (2.11.0-rc1): Cannot open git-sh-i18n
From: Junio C Hamano @ 2016-11-08 21:31 UTC (permalink / raw)
To: Stefan Beller; +Cc: Anthony Sottile, git@vger.kernel.org
In-Reply-To: <CAGZ79kbyM0ssz3JgeNHsUpvHnBsCbhm-tHvHRp3+6O1QvmkYpw@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Mon, Nov 7, 2016 at 10:30 AM, Anthony Sottile <asottile@umich.edu> wrote:
>> This has worked great up until now (and is very convenient for trying things
>> out without blowing away the system installation). What changed?
>>
>
> (Just guessing myself:)
>
> $ git log --grep git-sh-i18n v2.10.0..v2.11.0-rc0
> ...
> commit 1073094f30a8dd5ae49f2146f587085c4fe86410
> Author: Anders Kaseorg <andersk@mit.edu>
> Date: Sat Oct 29 22:10:02 2016 -0400
>
> git-sh-setup: be explicit where to dot-source git-sh-i18n from.
>
> d323c6b641 ("i18n: git-sh-setup.sh: mark strings for translation",
> 2016-06-17) started to dot-source git-sh-i18n shell script library,
> assuming that $PATH is already adjusted for our scripts, namely,
> $GIT_EXEC_PATH is at the beginning of $PATH.
> ...
Before that one since v2.10.0, there ie d323c6b641 that starts to
include git-sh-i18n from git-sh-setup in the first place. If you are
testing or using a newly-out-of-oven uninstalled Git, you would need
to do these to be correctly using it:
* Many subcommand executables and helpers are not installed on any
directory on your $PATH, but are installed in GIT_EXEC_PATH.
Find out where it is by asking the newly-out-of-oven Git "git
--exec-path" without setting GIT_EXEC_PATH environment variable.
That is where your newly built one wants to find things.
* Because you are trying to use the newly-out-of-oven Git without
installing, you do not want to install into the real
GIT_EXEC_PATH location yet. Pick a new empty directory and
arrange the files that would be installed by our Makefile into
"git --exec-path" if you did "make install" to be in this new
location. The set of files include git-sh-setup and git-sh-i18n.
* Set and export GIT_EXEC_PATH to point at this new empty directory
you just populated.
That incidentally is how we make our "make test" work.
If you only set $PATH to the top of git build directory, without
doing the above arrangement with GIT_EXEC_PATH, things may have
appeared to work due to multitude of accidents. Builtin commands
would worked fine because it is just a single "git" binary after
all, i.e. "git log" would not have consulted a "git-log" binary on
anywhere in your filesystem. Other commands may have been run from
the already installed version on the system. The latter is
particularly problematic, because it means that one thought that one
is testing the newly built on before installing to make sure that
the new one works OK, but is actually testing the already installed
one.
^ permalink raw reply
* Re: [PATCH v4 1/2] lib-proto-disable: variable name fix
From: Junio C Hamano @ 2016-11-08 20:52 UTC (permalink / raw)
To: Jacob Keller
Cc: Jeff King, Brandon Williams, Git mailing list, Stefan Beller,
bburky, Jonathan Nieder
In-Reply-To: <CA+P7+xohFeWb1hsOg1_T1kWenc=AKrt1TNcx=TrVYQ+w3+c63Q@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> On Mon, Nov 7, 2016 at 12:48 PM, Jeff King <peff@peff.net> wrote:
>> It's possible that I'm overly picky about my commit messages, but that
>> does not stop me from trying to train an army of picky-commit-message
>> clones. :)
>>
>> -Peff
>
> You're not the only one ;)
Somebody seems to have trained y'all very well ;-)
^ permalink raw reply
* Re: [PATCH 5/6] config docs: Provide for config to specify tags not to abbreviate
From: Junio C Hamano @ 2016-11-09 5:55 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jeff King, Jacob Keller, Git mailing list, Paul Mackerras
In-Reply-To: <22562.32428.287354.214659@chiark.greenend.org.uk>
Ian Jackson <ijackson@chiark.greenend.org.uk> writes:
>> I think the two things I found weird were:
>>
>> - it's in the "log" section, which makes me think it's an option for
>> git-log. But it's not. I'm not sure what the _right_ section is, but
>> hopefully it would make it clear that this is command-agnostic.
>>
>> Something like "gui.abbrevTags" might be OK (and as you note, has
>> precedence). But of course it's possible that a command like "tig"
>> could learn to support it. I'm not sure if that counts as a GUI or
>> not. :)
>
> I don't really have an opinion about the name. gui.abbrevTags would
> be a possibility. (It's a bit odd that implicitly, the default would
> be `*'.)
I have trouble with both "log" and "abbrev" in the name. Perhaps I
am biased by our recent discussion on a feature in the core that we
use the word "abbrev" to describe, but I fear that most Git users,
when told the word, would imagine the act of shortening 40-hex full
object name down to shorter but still unique prefix, not the "this
refname is too long, so let's show only the first few letters in GUI
label".
And I do not think we would want "log" or any core side Porcelain
command to have too many "information losing" options like this
"truncate refnames down to a point where it is no longer unique and
meaningful". GUI tools can get away with doing sos because they can
arrange these truncated labels to react to end-user input (e.g. the
truncated Tag in the history display of gitk could be made to react
to mouse-over and pop-up to show a full name, for example), but the
output from the core side is pretty much fixed once it is emitted.
So my first preference would be to teach gitk such a "please
clarify" UI-reaction, if it does not know how to do so yet. There
is no need for a configuration variable anywhere with this approach.
If you do want to add a configuration to show fuller name in the
tag, which would make it unnecessary for the user to do "please
clarify, as I am hovering over what I want to get details of"
action, that may also be a good way to go. But I think the right
place to do so would be Edit -> Preferences menu in Gitk, and the
settings will be stored in ~/.gitk or ~/.config/git/gitk or whatever
gitk-specific place.
^ permalink raw reply
* [PATCH] sequencer: silence -Wtautological-constant-out-of-range-compare
From: Jeff King @ 2016-11-09 3:57 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Lars Schneider
When clang compiles sequencer.c, it complains:
sequencer.c:632:14: warning: comparison of constant 2 with
expression of type 'const enum todo_command' is always
true [-Wtautological-constant-out-of-range-compare]
if (command < ARRAY_SIZE(todo_command_strings))
This is because "command" is an enum that may only have two
values (0 and 1) and the array in question has two elements.
As it turns out, clang is actually wrong here, at least
according to its own bug tracker:
https://llvm.org/bugs/show_bug.cgi?id=16154
But it's still worth working around this, as the warning is
present with -Wall, meaning we fail compilation with "make
DEVELOPER=1".
Casting the enum to size_t sufficiently unconfuses clang. As
a bonus, it also catches any possible out-of-bounds access
if the enum takes on a negative value (which shouldn't
happen either, but again, this is a defensive check).
Signed-off-by: Jeff King <peff@peff.net>
---
I know that a different fix is coming in a follow-on series, but I think
it's worth doing this to un-break clang on master (and v2.11) in the
meantime.
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 5fd75f30d..6f0ff9e41 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -629,7 +629,7 @@ static const char *todo_command_strings[] = {
static const char *command_to_string(const enum todo_command command)
{
- if (command < ARRAY_SIZE(todo_command_strings))
+ if ((size_t)command < ARRAY_SIZE(todo_command_strings))
return todo_command_strings[command];
die("Unknown command: %d", command);
}
--
2.11.0.rc0.263.g6f44bc3
^ permalink raw reply related
* Re: Bug: git config does not respect read-only .gitconfig file
From: Jeff King @ 2016-11-09 3:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Word, Markus Hitter, git, jword
In-Reply-To: <xmqqk2cdbg5v.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 08, 2016 at 05:22:52PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Probably converting "rename(from, to)" to first check "access(to,
> > W_OK)". That's racy, but it's the best we could do.
>
> Hmph, if these (possibly problematic) callers are all following the
> usual "lock, write to temp, rename" pattern, perhaps the lock_file()
> function can have access(path, W_OK) check before it returns a
> tempfile that has been successfully opened?
Yeah, that is a lot friendlier, as it prevents the caller from doing
work (which may even involve the user typing things!) when it is clear
that we would fail the final step anyway.
-Peff
^ permalink raw reply
* Re: [PATCH 5/6] config docs: Provide for config to specify tags not to abbreviate
From: Ian Jackson @ 2016-11-09 1:41 UTC (permalink / raw)
To: Jeff King; +Cc: Jacob Keller, Git mailing list, Junio C Hamano, Paul Mackerras
In-Reply-To: <20161108215709.rvmsnz4fvhizbocl@sigill.intra.peff.net>
Jeff King writes ("Re: [PATCH 5/6] config docs: Provide for config to specify tags not to abbreviate"):
> Yeah, I think git's config system was always designed to carry options
> for porcelains outside of git-core itself. So your new option fits into
> that.
Good, thanks.
> I think the two things I found weird were:
>
> - it's in the "log" section, which makes me think it's an option for
> git-log. But it's not. I'm not sure what the _right_ section is, but
> hopefully it would make it clear that this is command-agnostic.
>
> Something like "gui.abbrevTags" might be OK (and as you note, has
> precedence). But of course it's possible that a command like "tig"
> could learn to support it. I'm not sure if that counts as a GUI or
> not. :)
I don't really have an opinion about the name. gui.abbrevTags would
be a possibility. (It's a bit odd that implicitly, the default would
be `*'.)
> - The description talks about tag abbreviation, but doesn't really
> define it. Not being a gitk user, it was hard for me to figure out
> whether this was even relevant. Does it mean turning
> "refs/tags/v1.0" into "1.0"? From the rest of the series, it sounds
> like no. That should be more clear from the documentation.
I can do that, sure.
By default, gitk doesn't like to use much screen real estate for tags.
If there are long tag names, or many tags, it shows them all as a
single small indication saying just `<tag...|' or whatever with the
literal `tag...', not with the tag value.
Maybe a better name would be
gui.alwaysShowTags
?
I'm happy to be just told what the name ought to be, if the gitk and
git maintainers can agree. It seems largely a matter of taste.
Thanks,
Ian.
--
Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own.
If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
^ permalink raw reply
* Re: Bug: git config does not respect read-only .gitconfig file
From: Junio C Hamano @ 2016-11-09 1:22 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Word, Markus Hitter, git, jword
In-Reply-To: <20161108200110.zvqdm2nlu5zxfyv5@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Probably converting "rename(from, to)" to first check "access(to,
> W_OK)". That's racy, but it's the best we could do.
Hmph, if these (possibly problematic) callers are all following the
usual "lock, write to temp, rename" pattern, perhaps the lock_file()
function can have access(path, W_OK) check before it returns a
tempfile that has been successfully opened?
Having said that, I share your assessment that this is not a code or
design problem. It is unreasonable to drop the write-enable bit of
a file in a writable directory and expect it to stay unmodified. The
W-bit on the file is not usable as a security measure, and we do not
use it as such.
I do not offhand know how much a new feature "this repository can be
modified by pushing into and fetching from, but its configuration
cannot be modified" is a sensible thing to have. But it is quite
clear that even if we were to implement such feature, we wouldn't be
using W-bit on .git/config to signal that.
^ permalink raw reply
* Re: [PATCH v7 16/17] branch: use ref-filter printing APIs
From: Jacob Keller @ 2016-11-09 0:14 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-17-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Port branch.c to use ref-filter APIs for printing. This clears out
> most of the code used in branch.c for printing and replaces them with
> calls made to the ref-filter library.
Nice. This looks correct based on checking against the current
branch.c implementation by hand. There was one minor change I
suggested but I'm not really sure it buys is that much.
Thanks,
Jake
>
> Introduce build_format() which gets the format required for printing
> of refs. Make amendments to print_ref_list() to reflect these changes.
>
Ok.
> Change calc_maxwidth() to also account for the length of HEAD ref, by
> calling ref-filter:get_head_discription().
>
> Also change the test in t6040 to reflect the changes.
Right.
>
> Before this patch, all cross-prefix symrefs weren't shortened. Since
> we're using ref-filter APIs, we shorten all symrefs by default. We also
> allow the user to change the format if needed with the introduction of
> the '--format' option in the next patch.
>
This also makes sense.
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> builtin/branch.c | 234 ++++++++++++++---------------------------------
> t/t3203-branch-output.sh | 2 +-
> t/t6040-tracking-info.sh | 2 +-
> 3 files changed, 70 insertions(+), 168 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index dead2b8..a19e05d 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -36,12 +36,12 @@ static unsigned char head_sha1[20];
>
> static int branch_use_color = -1;
> static char branch_colors[][COLOR_MAXLEN] = {
> - GIT_COLOR_RESET,
> - GIT_COLOR_NORMAL, /* PLAIN */
> - GIT_COLOR_RED, /* REMOTE */
> - GIT_COLOR_NORMAL, /* LOCAL */
> - GIT_COLOR_GREEN, /* CURRENT */
> - GIT_COLOR_BLUE, /* UPSTREAM */
> + "%(color:reset)",
> + "%(color:reset)", /* PLAIN */
> + "%(color:red)", /* REMOTE */
> + "%(color:reset)", /* LOCAL */
> + "%(color:green)", /* CURRENT */
> + "%(color:blue)", /* UPSTREAM */
> };
> enum color_branch {
> BRANCH_COLOR_RESET = 0,
> @@ -280,162 +280,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> return(ret);
> }
>
> -static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
> - int show_upstream_ref)
> -{
> - int ours, theirs;
> - char *ref = NULL;
> - struct branch *branch = branch_get(branch_name);
> - const char *upstream;
> - struct strbuf fancy = STRBUF_INIT;
> - int upstream_is_gone = 0;
> - int added_decoration = 1;
> -
> - if (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {
> - if (!upstream)
> - return;
> - upstream_is_gone = 1;
> - }
> -
> - if (show_upstream_ref) {
> - ref = shorten_unambiguous_ref(upstream, 0);
> - if (want_color(branch_use_color))
> - strbuf_addf(&fancy, "%s%s%s",
> - branch_get_color(BRANCH_COLOR_UPSTREAM),
> - ref, branch_get_color(BRANCH_COLOR_RESET));
> - else
> - strbuf_addstr(&fancy, ref);
> - }
> -
> - if (upstream_is_gone) {
> - if (show_upstream_ref)
> - strbuf_addf(stat, _("[%s: gone]"), fancy.buf);
> - else
> - added_decoration = 0;
> - } else if (!ours && !theirs) {
> - if (show_upstream_ref)
> - strbuf_addf(stat, _("[%s]"), fancy.buf);
> - else
> - added_decoration = 0;
> - } else if (!ours) {
> - if (show_upstream_ref)
> - strbuf_addf(stat, _("[%s: behind %d]"), fancy.buf, theirs);
> - else
> - strbuf_addf(stat, _("[behind %d]"), theirs);
> -
> - } else if (!theirs) {
> - if (show_upstream_ref)
> - strbuf_addf(stat, _("[%s: ahead %d]"), fancy.buf, ours);
> - else
> - strbuf_addf(stat, _("[ahead %d]"), ours);
> - } else {
> - if (show_upstream_ref)
> - strbuf_addf(stat, _("[%s: ahead %d, behind %d]"),
> - fancy.buf, ours, theirs);
> - else
> - strbuf_addf(stat, _("[ahead %d, behind %d]"),
> - ours, theirs);
> - }
> - strbuf_release(&fancy);
> - if (added_decoration)
> - strbuf_addch(stat, ' ');
> - free(ref);
> -}
> -
> -static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
> - struct ref_filter *filter, const char *refname)
> -{
> - struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
> - const char *sub = _(" **** invalid ref ****");
> - struct commit *commit = item->commit;
> -
> - if (!parse_commit(commit)) {
> - pp_commit_easy(CMIT_FMT_ONELINE, commit, &subject);
> - sub = subject.buf;
> - }
> -
> - if (item->kind == FILTER_REFS_BRANCHES)
> - fill_tracking_info(&stat, refname, filter->verbose > 1);
> -
> - strbuf_addf(out, " %s %s%s",
> - find_unique_abbrev(item->commit->object.oid.hash, filter->abbrev),
> - stat.buf, sub);
> - strbuf_release(&stat);
> - strbuf_release(&subject);
> -}
> -
> -static void format_and_print_ref_item(struct ref_array_item *item, int maxwidth,
> - struct ref_filter *filter, const char *remote_prefix)
> -{
> - char c;
> - int current = 0;
> - int color;
> - struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
> - const char *prefix_to_show = "";
> - const char *prefix_to_skip = NULL;
> - const char *desc = item->refname;
> - char *to_free = NULL;
> -
> - switch (item->kind) {
> - case FILTER_REFS_BRANCHES:
> - prefix_to_skip = "refs/heads/";
> - skip_prefix(desc, prefix_to_skip, &desc);
> - if (!filter->detached && !strcmp(desc, head))
> - current = 1;
> - else
> - color = BRANCH_COLOR_LOCAL;
> - break;
> - case FILTER_REFS_REMOTES:
> - prefix_to_skip = "refs/remotes/";
> - skip_prefix(desc, prefix_to_skip, &desc);
> - color = BRANCH_COLOR_REMOTE;
> - prefix_to_show = remote_prefix;
> - break;
> - case FILTER_REFS_DETACHED_HEAD:
> - desc = to_free = get_head_description();
> - current = 1;
> - break;
> - default:
> - color = BRANCH_COLOR_PLAIN;
> - break;
> - }
> -
> - c = ' ';
> - if (current) {
> - c = '*';
> - color = BRANCH_COLOR_CURRENT;
> - }
> -
> - strbuf_addf(&name, "%s%s", prefix_to_show, desc);
> - if (filter->verbose) {
> - int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
> - strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
> - maxwidth + utf8_compensation, name.buf,
> - branch_get_color(BRANCH_COLOR_RESET));
> - } else
> - strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
> - name.buf, branch_get_color(BRANCH_COLOR_RESET));
> -
> - if (item->symref) {
> - const char *symref = item->symref;
> - if (prefix_to_skip)
> - skip_prefix(symref, prefix_to_skip, &symref);
> - strbuf_addf(&out, " -> %s", symref);
> - }
> - else if (filter->verbose)
> - /* " f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h" */
> - add_verbose_info(&out, item, filter, desc);
> - if (column_active(colopts)) {
> - assert(!filter->verbose && "--column and --verbose are incompatible");
> - string_list_append(&output, out.buf);
> - } else {
> - printf("%s\n", out.buf);
> - }
> - strbuf_release(&name);
> - strbuf_release(&out);
> - free(to_free);
> -}
> -
> static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
> {
> int i, max = 0;
> @@ -446,7 +290,12 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
>
> skip_prefix(it->refname, "refs/heads/", &desc);
> skip_prefix(it->refname, "refs/remotes/", &desc);
> - w = utf8_strwidth(desc);
> + if (it->kind == FILTER_REFS_DETACHED_HEAD) {
> + char *head_desc = get_head_description();
> + w = utf8_strwidth(head_desc);
> + free(head_desc);
> + } else
> + w = utf8_strwidth(desc);
>
> if (it->kind == FILTER_REFS_REMOTES)
> w += remote_bonus;
> @@ -456,12 +305,52 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
> return max;
> }
>
> +static char *build_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)
> +{
> + struct strbuf fmt = STRBUF_INIT;
> + struct strbuf local = STRBUF_INIT;
> + struct strbuf remote = STRBUF_INIT;
> +
Ok, so here we go for reviewing the new formats.
> + strbuf_addf(&fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)", branch_get_color(BRANCH_COLOR_CURRENT));
> +
The first thing we print, is the * when it's HEAD or two spaces,
followed by getting the current color scheme. Ok, this matches what we
do with branch today.
> + if (filter->verbose) {
> + strbuf_addf(&local, "%%(align:%d,left)%%(refname:strip=2)%%(end)", maxwidth);
> + strbuf_addf(&local, "%s", branch_get_color(BRANCH_COLOR_RESET));
> + strbuf_addf(&local, " %%(objectname:short=7) ");
> +
Now, for verbose mode we setup local to be left aligned to a maximum
width, with the refname stripped by 2 fields.
Then, we reset the color, and print the object name shorted to 7 characters. Ok.
> + if (filter->verbose > 1)
> + strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
> + "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
> + branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
When we have extra verbose, we check whether we have an upstream, and
if so, we print the short name of that upstream inside brackets. If we
have tracking information, we print that without brackets, and then we
end this section. Finally we print the subject.
We could almost re-use the code for the subject bits, but I'm not sure
it's worth it. Maybe drop the %contents:subject part and add it
afterwards since we always want it? It would remove some duplication
but overall not sure it's actually worth it.
> + else
> + strbuf_addf(&local, "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)");
> +
> + strbuf_addf(&remote, "%s%%(align:%d,left)%s%%(refname:strip=2)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
> + "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
> + branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,
> + remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
Here we handle the remote value, which is always the same for verbose
with either one or two value.
> + } else {
> + strbuf_addf(&local, "%%(refname:strip=2)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
> + branch_get_color(BRANCH_COLOR_RESET));
> + strbuf_addf(&remote, "%s%s%%(refname:strip=2)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
> + branch_get_color(BRANCH_COLOR_REMOTE), remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
> + }
> +
> + strbuf_addf(&fmt, "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)", local.buf, remote.buf);
> +
Finally we check to see whether the base is remotes and print the
local vs the remote buf. Neat trick.
> + strbuf_release(&local);
> + strbuf_release(&remote);
> + return strbuf_detach(&fmt, NULL);
> +}
> +
> static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
> {
> int i;
> struct ref_array array;
> int maxwidth = 0;
> const char *remote_prefix = "";
> + struct strbuf out = STRBUF_INIT;
> + char *format;
>
> /*
> * If we are listing more than just remote branches,
> @@ -473,12 +362,14 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>
> memset(&array, 0, sizeof(array));
>
> - verify_ref_format("%(refname)%(symref)");
> filter_refs(&array, filter, filter->kind | FILTER_REFS_INCLUDE_BROKEN);
>
> if (filter->verbose)
> maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
>
> + format = build_format(filter, maxwidth, remote_prefix);
> + verify_ref_format(format);
> +
> /*
> * If no sorting parameter is given then we default to sorting
> * by 'refname'. This would give us an alphabetically sorted
> @@ -490,10 +381,21 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
> sorting = ref_default_sorting();
> ref_array_sort(sorting, &array);
>
> - for (i = 0; i < array.nr; i++)
> - format_and_print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
> + for (i = 0; i < array.nr; i++) {
> + format_ref_array_item(array.items[i], format, 0, &out);
> + if (column_active(colopts)) {
> + assert(!filter->verbose && "--column and --verbose are incompatible");
> + /* format to a string_list to let print_columns() do its job */
> + string_list_append(&output, out.buf);
> + } else {
> + fwrite(out.buf, 1, out.len, stdout);
> + putchar('\n');
> + }
> + strbuf_release(&out);
> + }
>
> ref_array_clear(&array);
> + free(format);
> }
>
> static void reject_rebase_or_bisect_branch(const char *target)
> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index c6a3ccb..980c732 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -189,7 +189,7 @@ test_expect_success 'local-branch symrefs shortened properly' '
> git symbolic-ref refs/heads/ref-to-remote refs/remotes/origin/branch-one &&
> cat >expect <<-\EOF &&
> ref-to-branch -> branch-one
> - ref-to-remote -> refs/remotes/origin/branch-one
> + ref-to-remote -> origin/branch-one
> EOF
> git branch >actual.raw &&
> grep ref-to <actual.raw >actual &&
> diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
> index 3d5c238..97a0765 100755
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -44,7 +44,7 @@ b1 [ahead 1, behind 1] d
> b2 [ahead 1, behind 1] d
> b3 [behind 1] b
> b4 [ahead 2] f
> -b5 g
> +b5 [gone] g
> b6 c
> EOF
>
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 00/17] port branch.c to use ref-filter's printing options
From: Jacob Keller @ 2016-11-09 0:15 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-1-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> This is part of unification of the commands 'git tag -l, git branch -l
> and git for-each-ref'. This ports over branch.c to use ref-filter's
> printing options.
>
> Initially posted here: $(gmane/279226). It was decided that this series
> would follow up after refactoring ref-filter parsing mechanism, which
> is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
>
> v1 can be found here: $(gmane/288342)
> v2 can be found here: $(gmane/288863)
> v3 can be found here: $(gmane/290299)
> v4 can be found here: $(gmane/291106)
> v5b can be found here: $(gmane/292467)
> v6 can be found here: http://marc.info/?l=git&m=146330914118766&w=2
>
I reviewed the full series. I found a few minor things I would have
done differently, but overall I think it looks good. Thanks for the
hard work and the time invested here. I remember seeing this on the
list quite some time ago, so it's nice to see it finally come
together.
Thanks,
Jake
> Changes in this version:
>
> 1. Rebased on top of master.
>
> Karthik Nayak (17):
> ref-filter: implement %(if), %(then), and %(else) atoms
> ref-filter: include reference to 'used_atom' within 'atom_value'
> ref-filter: implement %(if:equals=<string>) and
> %(if:notequals=<string>)
> ref-filter: modify "%(objectname:short)" to take length
> ref-filter: move get_head_description() from branch.c
> ref-filter: introduce format_ref_array_item()
> ref-filter: make %(upstream:track) prints "[gone]" for invalid
> upstreams
> ref-filter: add support for %(upstream:track,nobracket)
> ref-filter: make "%(symref)" atom work with the ':short' modifier
> ref-filter: introduce refname_atom_parser_internal()
> ref-filter: introduce symref_atom_parser() and refname_atom_parser()
> ref-filter: make remote_ref_atom_parser() use
> refname_atom_parser_internal()
> ref-filter: add `:dir` and `:base` options for ref printing atoms
> ref-filter: allow porcelain to translate messages in the output
> branch, tag: use porcelain output
> branch: use ref-filter printing APIs
> branch: implement '--format' option
>
> Documentation/git-branch.txt | 7 +-
> Documentation/git-for-each-ref.txt | 82 ++++++-
> builtin/branch.c | 277 +++++++---------------
> builtin/tag.c | 2 +
> ref-filter.c | 456 +++++++++++++++++++++++++++++++------
> ref-filter.h | 7 +
> t/t3203-branch-output.sh | 16 +-
> t/t6040-tracking-info.sh | 2 +-
> t/t6300-for-each-ref.sh | 73 +++++-
> t/t6302-for-each-ref-filter.sh | 94 ++++++++
> 10 files changed, 725 insertions(+), 291 deletions(-)
>
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 15/17] branch, tag: use porcelain output
From: Jacob Keller @ 2016-11-09 0:01 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-16-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Call ref-filter's setup_ref_filter_porcelain_msg() to enable
> translated messages for the %(upstream:tack) atom. Although branch.c
> doesn't currently use ref-filter's printing API's, this will ensure
> that when it does in the future patches, we do not need to worry about
> translation.
>
Makes sense.
Thanks,
Jake
> Written-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> builtin/branch.c | 2 ++
> builtin/tag.c | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index be9773a..dead2b8 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -656,6 +656,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> OPT_END(),
> };
>
> + setup_ref_filter_porcelain_msg();
> +
> memset(&filter, 0, sizeof(filter));
> filter.kind = FILTER_REFS_BRANCHES;
> filter.abbrev = -1;
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 50e4ae5..a00e9a7 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -373,6 +373,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> OPT_END()
> };
>
> + setup_ref_filter_porcelain_msg();
> +
> git_config(git_tag_config, sorting_tail);
>
> memset(&opt, 0, sizeof(opt));
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 14/17] ref-filter: allow porcelain to translate messages in the output
From: Jacob Keller @ 2016-11-09 0:00 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-15-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Introduce setup_ref_filter_porcelain_msg() so that the messages used in
> the atom %(upstream:track) can be translated if needed. This is needed
> as we port branch.c to use ref-filter's printing API's.
>
So any user that wants these translated calls
setup_ref_filter_porcelain_msg but this will impact all callers from
that point on. Ok, I think that's ok? Otherwise they get default
without translation.
> Written-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> ref-filter.c | 28 ++++++++++++++++++++++++----
> ref-filter.h | 2 ++
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index b47b900..944671a 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -15,6 +15,26 @@
> #include "version.h"
> #include "wt-status.h"
>
> +static struct ref_msg {
> + const char *gone;
> + const char *ahead;
> + const char *behind;
> + const char *ahead_behind;
> +} msgs = {
> + "gone",
> + "ahead %d",
> + "behind %d",
> + "ahead %d, behind %d"
> +};
> +
> +void setup_ref_filter_porcelain_msg(void)
> +{
> + msgs.gone = _("gone");
> + msgs.ahead = _("ahead %d");
> + msgs.behind = _("behind %d");
> + msgs.ahead_behind = _("ahead %d, behind %d");
> +}
> +
> typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>
> struct align {
> @@ -1130,15 +1150,15 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
> else if (atom->u.remote_ref.option == RR_TRACK) {
> if (stat_tracking_info(branch, &num_ours,
> &num_theirs, NULL)) {
> - *s = xstrdup("gone");
> + *s = xstrdup(msgs.gone);
> } else if (!num_ours && !num_theirs)
> *s = "";
> else if (!num_ours)
> - *s = xstrfmt("behind %d", num_theirs);
> + *s = xstrfmt(msgs.behind, num_theirs);
> else if (!num_theirs)
> - *s = xstrfmt("ahead %d", num_ours);
> + *s = xstrfmt(msgs.ahead, num_ours);
> else
> - *s = xstrfmt("ahead %d, behind %d",
> + *s = xstrfmt(msgs.ahead_behind,
> num_ours, num_theirs);
> if (!atom->u.remote_ref.nobracket && *s[0]) {
> const char *to_free = *s;
> diff --git a/ref-filter.h b/ref-filter.h
> index 0014b92..da17145 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -111,5 +111,7 @@ struct ref_sorting *ref_default_sorting(void);
> int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
> /* Get the current HEAD's description */
> char *get_head_description(void);
> +/* Set up translated strings in the output. */
> +void setup_ref_filter_porcelain_msg(void);
>
> #endif /* REF_FILTER_H */
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Jacob Keller @ 2016-11-08 23:58 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-14-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Add the options `:dir` and `:base` to all ref printing ('%(refname)',
> '%(symref)', '%(push)' and '%(upstream)') atoms. The `:dir` option gives
> the directory (the part after $GIT_DIR/) of the ref without the
> refname. The `:base` option gives the base directory of the given
> ref (i.e. the directory following $GIT_DIR/refs/).
>
Nice, this seems useful.
> Add tests and documentation for the same.
>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> Documentation/git-for-each-ref.txt | 34 +++++++++++++++++++---------------
> ref-filter.c | 29 +++++++++++++++++++++++++----
> t/t6300-for-each-ref.sh | 24 ++++++++++++++++++++++++
> 3 files changed, 68 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 600b703..f4ad297 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -96,7 +96,9 @@ refname::
> slash-separated path components from the front of the refname
> (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
> `<N>` must be a positive integer. If a displayed ref has fewer
> - components than `<N>`, the command aborts with an error.
> + components than `<N>`, the command aborts with an error. For the base
> + directory of the ref (i.e. foo in refs/foo/bar/boz) append
> + `:base`. For the entire directory path append `:dir`.
>
> objecttype::
> The type of the object (`blob`, `tree`, `commit`, `tag`).
> @@ -114,22 +116,23 @@ objectname::
>
> upstream::
> The name of a local ref which can be considered ``upstream''
> - from the displayed ref. Respects `:short` and `:strip` in the
> - same way as `refname` above. Additionally respects `:track`
> - to show "[ahead N, behind M]" and `:trackshort` to show the
> - terse version: ">" (ahead), "<" (behind), "<>" (ahead and
> - behind), or "=" (in sync). `:track` also prints "[gone]"
> - whenever unknown upstream ref is encountered. Append
> - `:track,nobracket` to show tracking information without
> - brackets (i.e "ahead N, behind M"). Has no effect if the ref
> - does not have tracking information associated with it.
> + from the displayed ref. Respects `:short`, `:strip`, `:base`
> + and `:dir` in the same way as `refname` above. Additionally
> + respects `:track` to show "[ahead N, behind M]" and
> + `:trackshort` to show the terse version: ">" (ahead), "<"
> + (behind), "<>" (ahead and behind), or "=" (in sync). `:track`
> + also prints "[gone]" whenever unknown upstream ref is
> + encountered. Append `:track,nobracket` to show tracking
> + information without brackets (i.e "ahead N, behind M"). Has
> + no effect if the ref does not have tracking information
> + associated with it.
>
> push::
> The name of a local ref which represents the `@{push}`
> location for the displayed ref. Respects `:short`, `:strip`,
> - `:track`, and `:trackshort` options as `upstream`
> - does. Produces an empty string if no `@{push}` ref is
> - configured.
> + `:track`, `:trackshort`, `:base` and `:dir` options as
> + `upstream` does. Produces an empty string if no `@{push}` ref
> + is configured.
>
At this point would it make more sense to document the extra values
here in one block separately? For example, the upstream atom is
getting pretty complex with all those options. Additionally, some of
the options can be combined, like nobracket, but others cannot be
comined so It may be worth documenting how and when those combinations
work?
> HEAD::
> '*' if HEAD matches current ref (the checked out branch), ' '
> @@ -169,8 +172,9 @@ if::
>
> symref::
> The ref which the given symbolic ref refers to. If not a
> - symbolic ref, nothing is printed. Respects the `:short` and
> - `:strip` options in the same way as `refname` above.
> + symbolic ref, nothing is printed. Respects the `:short`,
> + `:strip`, `:base` and `:dir` options in the same way as
> + `refname` above.
>
> In addition to the above, for commit and tag objects, the header
> field names (`tree`, `parent`, `object`, `type`, and `tag`) can
> diff --git a/ref-filter.c b/ref-filter.c
> index 7d3d3a6..b47b900 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -31,7 +31,7 @@ struct if_then_else {
> };
>
> struct refname_atom {
> - enum { R_NORMAL, R_SHORT, R_STRIP } option;
> + enum { R_BASE, R_DIR, R_NORMAL, R_SHORT, R_STRIP } option;
> unsigned int strip;
> };
>
> @@ -93,7 +93,11 @@ static void refname_atom_parser_internal(struct refname_atom *atom,
> atom->option = R_STRIP;
> if (strtoul_ui(arg, 10, &atom->strip) || atom->strip <= 0)
> die(_("positive value expected refname:strip=%s"), arg);
> - } else
> + } else if (!strcmp(arg, "dir"))
> + atom->option = R_DIR;
> + else if (!strcmp(arg, "base"))
> + atom->option = R_BASE;
> + else
> die(_("unrecognized %%(%s) argument: %s"), name, arg);
> }
>
> @@ -252,7 +256,6 @@ static void if_atom_parser(struct used_atom *atom, const char *arg)
> die(_("unrecognized %%(if) argument: %s"), arg);
> }
>
> -
> static struct {
> const char *name;
> cmp_type cmp_type;
> @@ -1096,7 +1099,25 @@ static const char *show_ref(struct refname_atom *atom, const char *refname)
> return shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> else if (atom->option == R_STRIP)
> return strip_ref_components(refname, atom->strip);
> - else
> + else if (atom->option == R_BASE) {
> + const char *sp, *ep;
> +
> + if (skip_prefix(refname, "refs/", &sp)) {
> + ep = strchr(sp, '/');
> + if (!ep)
> + return "";
> + return xstrndup(sp, ep - sp);
> + }
> + return "";
> + } else if (atom->option == R_DIR) {
> + const char *sp, *ep;
> +
> + sp = refname;
> + ep = strrchr(sp, '/');
> + if (!ep)
> + return "";
> + return xstrndup(sp, ep - sp);
> + } else
> return refname;
> }
>
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 7ca0a12..8ff6568 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -53,12 +53,18 @@ test_atom head refname refs/heads/master
> test_atom head refname:short master
> test_atom head refname:strip=1 heads/master
> test_atom head refname:strip=2 master
> +test_atom head refname:dir refs/heads
> +test_atom head refname:base heads
> test_atom head upstream refs/remotes/origin/master
> test_atom head upstream:short origin/master
> test_atom head upstream:strip=2 origin/master
> +test_atom head upstream:dir refs/remotes/origin
> +test_atom head upstream:base remotes
> test_atom head push refs/remotes/myfork/master
> test_atom head push:short myfork/master
> test_atom head push:strip=1 remotes/myfork/master
> +test_atom head push:dir refs/remotes/myfork
> +test_atom head push:base remotes
> test_atom head objecttype commit
> test_atom head objectsize 171
> test_atom head objectname $(git rev-parse refs/heads/master)
> @@ -600,4 +606,22 @@ test_expect_success 'Verify usage of %(symref:strip) atom' '
> test_cmp expected actual
> '
>
> +cat >expected <<EOF
> +refs/heads
> +EOF
> +
> +test_expect_success 'Verify usage of %(symref:dir) atom' '
> + git for-each-ref --format="%(symref:dir)" refs/heads/sym > actual &&
> + test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
> +heads
> +EOF
> +
> +test_expect_success 'Verify usage of %(symref:base) atom' '
> + git for-each-ref --format="%(symref:base)" refs/heads/sym > actual &&
> + test_cmp expected actual
> +'
> +
> test_done
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 12/17] ref-filter: make remote_ref_atom_parser() use refname_atom_parser_internal()
From: Jacob Keller @ 2016-11-08 23:54 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-13-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Use the recently introduced refname_atom_parser_internal() within
> remote_ref_atom_parser(), this provides a common base for all the ref
> printing atoms, allowing %(upstream) and %(push) to also use the
> ':strip' option.
>
> The atoms '%(push)' and '%(upstream)' will retain the ':track' and
> ':trackshort' atom modifiers to themselves as they have no meaning in
> context to the '%(refname)' and '%(symref)' atoms.
>
> Update the documentation and tests to reflect the same.
>
Nice. Good to have all this become common.
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> Documentation/git-for-each-ref.txt | 27 ++++++++++++++-------------
> ref-filter.c | 26 +++++++++++++++-----------
> t/t6300-for-each-ref.sh | 2 ++
> 3 files changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index a669a32..600b703 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -114,21 +114,22 @@ objectname::
>
> upstream::
> The name of a local ref which can be considered ``upstream''
> - from the displayed ref. Respects `:short` in the same way as
> - `refname` above. Additionally respects `:track` to show
> - "[ahead N, behind M]" and `:trackshort` to show the terse
> - version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
> - or "=" (in sync). `:track` also prints "[gone]" whenever
> - unknown upstream ref is encountered. Append `:track,nobracket`
> - to show tracking information without brackets (i.e "ahead N,
> - behind M"). Has no effect if the ref does not have tracking
> - information associated with it.
> + from the displayed ref. Respects `:short` and `:strip` in the
> + same way as `refname` above. Additionally respects `:track`
> + to show "[ahead N, behind M]" and `:trackshort` to show the
> + terse version: ">" (ahead), "<" (behind), "<>" (ahead and
> + behind), or "=" (in sync). `:track` also prints "[gone]"
> + whenever unknown upstream ref is encountered. Append
> + `:track,nobracket` to show tracking information without
> + brackets (i.e "ahead N, behind M"). Has no effect if the ref
> + does not have tracking information associated with it.
>
> push::
> - The name of a local ref which represents the `@{push}` location
> - for the displayed ref. Respects `:short`, `:track`, and
> - `:trackshort` options as `upstream` does. Produces an empty
> - string if no `@{push}` ref is configured.
> + The name of a local ref which represents the `@{push}`
> + location for the displayed ref. Respects `:short`, `:strip`,
> + `:track`, and `:trackshort` options as `upstream`
> + does. Produces an empty string if no `@{push}` ref is
> + configured.
>
> HEAD::
> '*' if HEAD matches current ref (the checked out branch), ' '
> diff --git a/ref-filter.c b/ref-filter.c
> index f1d27b5..7d3d3a6 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -52,7 +52,8 @@ static struct used_atom {
> char color[COLOR_MAXLEN];
> struct align align;
> struct {
> - enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT } option;
> + enum { RR_REF, RR_TRACK, RR_TRACKSHORT } option;
> + struct refname_atom refname;
> unsigned int nobracket: 1;
> } remote_ref;
> struct {
> @@ -102,7 +103,9 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
> int i;
>
> if (!arg) {
> - atom->u.remote_ref.option = RR_NORMAL;
> + atom->u.remote_ref.option = RR_REF;
> + refname_atom_parser_internal(&atom->u.remote_ref.refname,
> + arg, atom->name);
> return;
> }
>
> @@ -112,16 +115,17 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
> for (i = 0; i < params.nr; i++) {
> const char *s = params.items[i].string;
>
> - if (!strcmp(s, "short"))
> - atom->u.remote_ref.option = RR_SHORTEN;
> - else if (!strcmp(s, "track"))
> + if (!strcmp(s, "track"))
> atom->u.remote_ref.option = RR_TRACK;
> else if (!strcmp(s, "trackshort"))
> atom->u.remote_ref.option = RR_TRACKSHORT;
> else if (!strcmp(s, "nobracket"))
> atom->u.remote_ref.nobracket = 1;
> - else
> - die(_("unrecognized format: %%(%s)"), atom->name);
> + else {
> + atom->u.remote_ref.option = RR_REF;
> + refname_atom_parser_internal(&atom->u.remote_ref.refname,
> + arg, atom->name);
> + }
> }
>
> string_list_clear(¶ms, 0);
> @@ -1100,8 +1104,8 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
> struct branch *branch, const char **s)
> {
> int num_ours, num_theirs;
> - if (atom->u.remote_ref.option == RR_SHORTEN)
> - *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> + if (atom->u.remote_ref.option == RR_REF)
> + *s = show_ref(&atom->u.remote_ref.refname, refname);
> else if (atom->u.remote_ref.option == RR_TRACK) {
> if (stat_tracking_info(branch, &num_ours,
> &num_theirs, NULL)) {
> @@ -1133,8 +1137,8 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
> *s = ">";
> else
> *s = "<>";
> - } else /* RR_NORMAL */
> - *s = refname;
> + } else
> + die("BUG: unhandled RR_* enum");
> }
>
> char *get_head_description(void)
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 3d28234..7ca0a12 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -55,8 +55,10 @@ test_atom head refname:strip=1 heads/master
> test_atom head refname:strip=2 master
> test_atom head upstream refs/remotes/origin/master
> test_atom head upstream:short origin/master
> +test_atom head upstream:strip=2 origin/master
> test_atom head push refs/remotes/myfork/master
> test_atom head push:short myfork/master
> +test_atom head push:strip=1 remotes/myfork/master
> test_atom head objecttype commit
> test_atom head objectsize 171
> test_atom head objectname $(git rev-parse refs/heads/master)
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 11/17] ref-filter: introduce symref_atom_parser() and refname_atom_parser()
From: Jacob Keller @ 2016-11-08 23:52 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-12-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Using refname_atom_parser_internal(), introduce symref_atom_parser() and
> refname_atom_parser() which will parse the atoms %(symref) and
> %(refname) respectively. Store the parsed information into the
> 'used_atom' structure based on the modifiers used along with the atoms.
>
> Now the '%(symref)' atom supports the ':strip' atom modifier. Update the
> Documentation and tests to reflect this.
>
One minor nit is that the first part is actually identical so I wonder
if it's worth having two separate functions?
Thanks,
Jake
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> Documentation/git-for-each-ref.txt | 5 +++
> ref-filter.c | 78 ++++++++++++++++++++++----------------
> t/t6300-for-each-ref.sh | 9 +++++
> 3 files changed, 59 insertions(+), 33 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 3953431..a669a32 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -166,6 +166,11 @@ if::
> the value between the %(if:...) and %(then) atoms with the
> given string.
>
> +symref::
> + The ref which the given symbolic ref refers to. If not a
> + symbolic ref, nothing is printed. Respects the `:short` and
> + `:strip` options in the same way as `refname` above.
> +
> In addition to the above, for commit and tag objects, the header
> field names (`tree`, `parent`, `object`, `type`, and `tag`) can
> be used to specify the value in the header field.
> diff --git a/ref-filter.c b/ref-filter.c
> index aad537d..f1d27b5 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -176,6 +176,16 @@ static void objectname_atom_parser(struct used_atom *atom, const char *arg)
> die(_("unrecognized %%(objectname) argument: %s"), arg);
> }
>
> +static void symref_atom_parser(struct used_atom *atom, const char *arg)
> +{
> + return refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
> +}
> +
> +static void refname_atom_parser(struct used_atom *atom, const char *arg)
> +{
> + return refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
> +}
> +
What's the reasoning for using separate functions here if they are
exactly identical except for name? Do we intend to add separate
options for this? I don't really have a problem with separate
functions here since it helps avoid confusion but they are identical
otherwise...
> static align_type parse_align_position(const char *s)
> {
> if (!strcmp(s, "right"))
> @@ -244,7 +254,7 @@ static struct {
> cmp_type cmp_type;
> void (*parser)(struct used_atom *atom, const char *arg);
> } valid_atom[] = {
> - { "refname" },
> + { "refname" , FIELD_STR, refname_atom_parser },
> { "objecttype" },
> { "objectsize", FIELD_ULONG },
> { "objectname", FIELD_STR, objectname_atom_parser },
> @@ -273,7 +283,7 @@ static struct {
> { "contents", FIELD_STR, contents_atom_parser },
> { "upstream", FIELD_STR, remote_ref_atom_parser },
> { "push", FIELD_STR, remote_ref_atom_parser },
> - { "symref" },
> + { "symref", FIELD_STR, symref_atom_parser },
> { "flag" },
> { "HEAD" },
> { "color", FIELD_STR, color_atom_parser },
> @@ -1058,21 +1068,16 @@ static inline char *copy_advance(char *dst, const char *src)
> return dst;
> }
>
> -static const char *strip_ref_components(const char *refname, const char *nr_arg)
> +static const char *strip_ref_components(const char *refname, unsigned int len)
> {
> - char *end;
> - long nr = strtol(nr_arg, &end, 10);
> - long remaining = nr;
> + long remaining = len;
> const char *start = refname;
>
> - if (nr < 1 || *end != '\0')
> - die(_(":strip= requires a positive integer argument"));
> -
> while (remaining) {
> switch (*start++) {
> case '\0':
> - die(_("ref '%s' does not have %ld components to :strip"),
> - refname, nr);
> + die(_("ref '%s' does not have %ud components to :strip"),
> + refname, len);
> case '/':
> remaining--;
> break;
> @@ -1081,6 +1086,16 @@ static const char *strip_ref_components(const char *refname, const char *nr_arg)
> return start;
> }
>
> +static const char *show_ref(struct refname_atom *atom, const char *refname)
> +{
> + if (atom->option == R_SHORT)
> + return shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> + else if (atom->option == R_STRIP)
> + return strip_ref_components(refname, atom->strip);
> + else
> + return refname;
> +}
> +
> static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
> struct branch *branch, const char **s)
> {
> @@ -1153,6 +1168,21 @@ char *get_head_description(void)
> return strbuf_detach(&desc, NULL);
> }
>
> +static const char *get_symref(struct used_atom *atom, struct ref_array_item *ref)
> +{
> + if (!ref->symref)
> + return "";
> + else
> + return show_ref(&atom->u.refname, ref->symref);
> +}
> +
> +static const char *get_refname(struct used_atom *atom, struct ref_array_item *ref)
> +{
> + if (ref->kind & FILTER_REFS_DETACHED_HEAD)
> + return get_head_description();
> + return show_ref(&atom->u.refname, ref->refname);
> +}
> +
> /*
> * Parse the object referred by ref, and grab needed value.
> */
> @@ -1181,7 +1211,6 @@ static void populate_value(struct ref_array_item *ref)
> struct atom_value *v = &ref->value[i];
> int deref = 0;
> const char *refname;
> - const char *formatp;
> struct branch *branch = NULL;
>
> v->handler = append_atom;
> @@ -1192,12 +1221,10 @@ static void populate_value(struct ref_array_item *ref)
> name++;
> }
>
> - if (starts_with(name, "refname")) {
> - refname = ref->refname;
> - if (ref->kind & FILTER_REFS_DETACHED_HEAD)
> - refname = get_head_description();
> - } else if (starts_with(name, "symref"))
> - refname = ref->symref ? ref->symref : "";
> + if (starts_with(name, "refname"))
> + refname = get_refname(atom, ref);
> + else if (starts_with(name, "symref"))
> + refname = get_symref(atom, ref);
> else if (starts_with(name, "upstream")) {
> const char *branch_name;
> /* only local branches may have an upstream */
> @@ -1273,21 +1300,6 @@ static void populate_value(struct ref_array_item *ref)
> } else
> continue;
>
> - formatp = strchr(name, ':');
> - if (formatp) {
> - const char *arg;
> -
> - formatp++;
> - if (!strcmp(formatp, "short"))
> - refname = shorten_unambiguous_ref(refname,
> - warn_ambiguous_refs);
> - else if (skip_prefix(formatp, "strip=", &arg))
> - refname = strip_ref_components(refname, arg);
> - else
> - die(_("unknown %.*s format %s"),
> - (int)(formatp - name), name, formatp);
> - }
> -
> if (!deref)
> v->s = refname;
> else
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index b06ea1c..3d28234 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -589,4 +589,13 @@ test_expect_success 'Verify usage of %(symref:short) atom' '
> test_cmp expected actual
> '
>
> +cat >expected <<EOF
> +master
> +EOF
> +
> +test_expect_success 'Verify usage of %(symref:strip) atom' '
> + git for-each-ref --format="%(symref:strip=2)" refs/heads/sym > actual &&
> + test_cmp expected actual
> +'
> +
> test_done
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 09/17] ref-filter: make "%(symref)" atom work with the ':short' modifier
From: Jacob Keller @ 2016-11-08 23:46 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-10-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> The "%(symref)" atom doesn't work when used with the ':short' modifier
> because we strictly match only 'symref' for setting the 'need_symref'
> indicator. Fix this by using comparing with valid_atom rather than used_atom.
>
Makes sense.
> Add tests for %(symref) and %(symref:short) while we're here.
>
Nice to see more tests around this.
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> ref-filter.c | 2 +-
> t/t6300-for-each-ref.sh | 24 ++++++++++++++++++++++++
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 4d7e414..5666814 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -338,7 +338,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
> valid_atom[i].parser(&used_atom[at], arg);
> if (*atom == '*')
> need_tagged = 1;
> - if (!strcmp(used_atom[at].name, "symref"))
> + if (!strcmp(valid_atom[i].name, "symref"))
> need_symref = 1;
> return at;
> }
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 2c5f177..b06ea1c 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -38,6 +38,7 @@ test_atom() {
> case "$1" in
> head) ref=refs/heads/master ;;
> tag) ref=refs/tags/testtag ;;
> + sym) ref=refs/heads/sym ;;
> *) ref=$1 ;;
> esac
> printf '%s\n' "$3" >expected
> @@ -565,4 +566,27 @@ test_expect_success 'Verify sort with multiple keys' '
> refs/tags/bogo refs/tags/master > actual &&
> test_cmp expected actual
> '
> +
> +test_expect_success 'Add symbolic ref for the following tests' '
> + git symbolic-ref refs/heads/sym refs/heads/master
> +'
> +
> +cat >expected <<EOF
> +refs/heads/master
> +EOF
> +
> +test_expect_success 'Verify usage of %(symref) atom' '
> + git for-each-ref --format="%(symref)" refs/heads/sym > actual &&
> + test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
> +heads/master
> +EOF
> +
> +test_expect_success 'Verify usage of %(symref:short) atom' '
> + git for-each-ref --format="%(symref:short)" refs/heads/sym > actual &&
> + test_cmp expected actual
> +'
> +
> test_done
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 08/17] ref-filter: add support for %(upstream:track,nobracket)
From: Jacob Keller @ 2016-11-08 23:45 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-9-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Add support for %(upstream:track,nobracket) which will print the
> tracking information without the brackets (i.e. "ahead N, behind M").
> This is needed when we port branch.c to use ref-filter's printing APIs.
>
Makes sense. Seems a bit weird that we have the brackets normally
rather than adding them as an option, but I think this is ok. We don't
want to change all previous uses in this case.
My only suggestion here would be to add code so that the options die()
when we use nobracket along with trackshort or without track. This
ensures that the nobracket option only applies to track mode?
> Add test and documentation for the same.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> Documentation/git-for-each-ref.txt | 8 +++--
> ref-filter.c | 67 +++++++++++++++++++++++++-------------
> t/t6300-for-each-ref.sh | 2 ++
> 3 files changed, 51 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index fd365eb..3953431 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -118,9 +118,11 @@ upstream::
> `refname` above. Additionally respects `:track` to show
> "[ahead N, behind M]" and `:trackshort` to show the terse
> version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
> - or "=" (in sync). Has no effect if the ref does not have
> - tracking information associated with it. `:track` also prints
> - "[gone]" whenever unknown upstream ref is encountered.
> + or "=" (in sync). `:track` also prints "[gone]" whenever
> + unknown upstream ref is encountered. Append `:track,nobracket`
> + to show tracking information without brackets (i.e "ahead N,
> + behind M"). Has no effect if the ref does not have tracking
> + information associated with it.
>
Ok so my comment on the previous patch is fixed here, the new wording
makes it much more clear that [gone] is not the same thing as no
information. So I don't think we should bother changing the previous
patch in the series. This might want to document that nobracket works
even without track, even if it doesn't actually do anything? Or make
the code more strict in that we die() if the values are put together
that make no sense?
> push::
> The name of a local ref which represents the `@{push}` location
> diff --git a/ref-filter.c b/ref-filter.c
> index 6d51b80..4d7e414 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -46,8 +46,10 @@ static struct used_atom {
> union {
> char color[COLOR_MAXLEN];
> struct align align;
> - enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
> - remote_ref;
> + struct {
> + enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT } option;
> + unsigned int nobracket: 1;
> + } remote_ref;
> struct {
> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
> unsigned int nlines;
> @@ -75,16 +77,33 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
>
> static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
> {
> - if (!arg)
> - atom->u.remote_ref = RR_NORMAL;
> - else if (!strcmp(arg, "short"))
> - atom->u.remote_ref = RR_SHORTEN;
> - else if (!strcmp(arg, "track"))
> - atom->u.remote_ref = RR_TRACK;
> - else if (!strcmp(arg, "trackshort"))
> - atom->u.remote_ref = RR_TRACKSHORT;
> - else
> - die(_("unrecognized format: %%(%s)"), atom->name);
> + struct string_list params = STRING_LIST_INIT_DUP;
> + int i;
> +
> + if (!arg) {
> + atom->u.remote_ref.option = RR_NORMAL;
> + return;
> + }
> +
> + atom->u.remote_ref.nobracket = 0;
> + string_list_split(¶ms, arg, ',', -1);
> +
> + for (i = 0; i < params.nr; i++) {
> + const char *s = params.items[i].string;
> +
> + if (!strcmp(s, "short"))
> + atom->u.remote_ref.option = RR_SHORTEN;
> + else if (!strcmp(s, "track"))
Should we add die()s here to disallow setting the remote_ref option
multiple times? Otherwise, we should document that the last one wins?
Not sure it's really a big deal here, but we could do it to ensure
consistency for options which don't make sense together?
> + atom->u.remote_ref.option = RR_TRACK;
> + else if (!strcmp(s, "trackshort"))
> + atom->u.remote_ref.option = RR_TRACKSHORT;
> + else if (!strcmp(s, "nobracket"))
> + atom->u.remote_ref.nobracket = 1;
> + else
> + die(_("unrecognized format: %%(%s)"), atom->name);
> + }
> +
> + string_list_clear(¶ms, 0);
> }
>
> static void body_atom_parser(struct used_atom *atom, const char *arg)
> @@ -1045,25 +1064,27 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
> struct branch *branch, const char **s)
> {
> int num_ours, num_theirs;
> - if (atom->u.remote_ref == RR_SHORTEN)
> + if (atom->u.remote_ref.option == RR_SHORTEN)
> *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> - else if (atom->u.remote_ref == RR_TRACK) {
> + else if (atom->u.remote_ref.option == RR_TRACK) {
> if (stat_tracking_info(branch, &num_ours,
> &num_theirs, NULL)) {
> - *s = "[gone]";
> - return;
> - }
> -
> - if (!num_ours && !num_theirs)
> + *s = xstrdup("gone");
> + } else if (!num_ours && !num_theirs)
> *s = "";
> else if (!num_ours)
> - *s = xstrfmt("[behind %d]", num_theirs);
> + *s = xstrfmt("behind %d", num_theirs);
> else if (!num_theirs)
> - *s = xstrfmt("[ahead %d]", num_ours);
> + *s = xstrfmt("ahead %d", num_ours);
> else
> - *s = xstrfmt("[ahead %d, behind %d]",
> + *s = xstrfmt("ahead %d, behind %d",
> num_ours, num_theirs);
> - } else if (atom->u.remote_ref == RR_TRACKSHORT) {
> + if (!atom->u.remote_ref.nobracket && *s[0]) {
> + const char *to_free = *s;
> + *s = xstrfmt("[%s]", *s);
> + free((void *)to_free);
> + }
> + } else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
> if (stat_tracking_info(branch, &num_ours,
> &num_theirs, NULL))
> return;
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index a92b36f..2c5f177 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -372,6 +372,8 @@ test_expect_success 'setup for upstream:track[short]' '
>
> test_atom head upstream:track '[ahead 1]'
> test_atom head upstream:trackshort '>'
> +test_atom head upstream:track,nobracket 'ahead 1'
> +test_atom head upstream:nobracket,track 'ahead 1'
> test_atom head push:track '[ahead 1]'
> test_atom head push:trackshort '>'
>
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 07/17] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
From: Jacob Keller @ 2016-11-08 23:37 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-8-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Borrowing from branch.c's implementation print "[gone]" whenever an
> unknown upstream ref is encountered instead of just ignoring it.
>
This makes sense.
> This makes sure that when branch.c is ported over to using ref-filter
> APIs for printing, this feature is not lost.
>
Right.
> Make changes to t/t6300-for-each-ref.sh and
> Documentation/git-for-each-ref.txt to reflect this change.
>
This will change behavior if people were expecting it to remain
silent, but I think this could be considered a bug.
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Helped-by : Jacob Keller <jacob.keller@gmail.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> Documentation/git-for-each-ref.txt | 3 ++-
> ref-filter.c | 4 +++-
> t/t6300-for-each-ref.sh | 2 +-
> 3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 92184c4..fd365eb 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -119,7 +119,8 @@ upstream::
> "[ahead N, behind M]" and `:trackshort` to show the terse
> version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
> or "=" (in sync). Has no effect if the ref does not have
> - tracking information associated with it.
> + tracking information associated with it. `:track` also prints
> + "[gone]" whenever unknown upstream ref is encountered.
>
I think this is poorly worded. If I understand, "has no effect if the
ref does not have tracking information" so in that case we still print
nothing, right? but otherwise we print [gone] only when the upstream
ref no longer actually exists locally? I wonder if there is a better
wording for this? I don't have one. Any suggestions to avoid confusing
these two cases?
> push::
> The name of a local ref which represents the `@{push}` location
> diff --git a/ref-filter.c b/ref-filter.c
> index b8b8a95..6d51b80 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1049,8 +1049,10 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
> *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> else if (atom->u.remote_ref == RR_TRACK) {
> if (stat_tracking_info(branch, &num_ours,
> - &num_theirs, NULL))
> + &num_theirs, NULL)) {
> + *s = "[gone]";
> return;
> + }
>
> if (!num_ours && !num_theirs)
> *s = "";
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 2be0a3f..a92b36f 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -382,7 +382,7 @@ test_expect_success 'Check that :track[short] cannot be used with other atoms' '
>
> test_expect_success 'Check that :track[short] works when upstream is invalid' '
> cat >expected <<-\EOF &&
> -
> + [gone]
>
> EOF
> test_when_finished "git config branch.master.merge refs/heads/master" &&
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 06/17] ref-filter: introduce format_ref_array_item()
From: Jacob Keller @ 2016-11-08 23:32 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-7-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> To allow column display, we will need to first render the output in a
> string list to allow print_columns() to compute the proper size of
> each column before starting the actual output. Introduce the function
> format_ref_array_item() that does the formatting of a ref_array_item
> to an strbuf.
>
Makes sense.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH v7 05/17] ref-filter: move get_head_description() from branch.c
From: Jacob Keller @ 2016-11-08 23:31 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-6-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Move the implementation of get_head_description() from branch.c to
> ref-filter. This gives a description of the HEAD ref if called. This
> is used as the refname for the HEAD ref whenever the
> FILTER_REFS_DETACHED_HEAD option is used. Make it public because we
> need it to calculate the length of the HEAD refs description in
> branch.c:calc_maxwidth() when we port branch.c to use ref-filter
> APIs.
>
Makes sense.
>
> - if (starts_with(name, "refname"))
> + if (starts_with(name, "refname")) {
> refname = ref->refname;
> - else if (starts_with(name, "symref"))
> + if (ref->kind & FILTER_REFS_DETACHED_HEAD)
> + refname = get_head_description();
Since this (I think?) changes behavior of refname would it make sense
to add a test for this?
Thanks,
Jake
> + } else if (starts_with(name, "symref"))
> refname = ref->symref ? ref->symref : "";
> else if (starts_with(name, "upstream")) {
> const char *branch_name;
> diff --git a/ref-filter.h b/ref-filter.h
> index 14d435e..4aea594 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -106,5 +106,7 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
> struct ref_sorting *ref_default_sorting(void);
> /* Function to parse --merged and --no-merged options */
> int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
> +/* Get the current HEAD's description */
> +char *get_head_description(void);
>
> #endif /* REF_FILTER_H */
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 04/17] ref-filter: modify "%(objectname:short)" to take length
From: Jacob Keller @ 2016-11-08 23:27 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-5-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Add support for %(objectname:short=<length>) which would print the
> abbreviated unique objectname of given length. When no length is
> specified, the length is 'DEFAULT_ABBREV'. The minimum length is
> 'MINIMUM_ABBREV'. The length may be exceeded to ensure that the provided
> object name is unique.
>
Ok this makes sense. It may be annoying that the length might go
beyond the size that we wanted, but I think it's better than printing
a non-unique short abbreviation.
I have one suggested change, which is to drop O_LENGTH and have
O_SHORT store the length always, setting it to DEFAULT_ABBREV when no
length provided. This allows you to drop some code. I don't think it's
actually worth a re-roll by itself since the current code is correct.
Thanks,
Jake
> Add tests and documentation for the same.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Helped-by: Jacob Keller <jacob.keller@gmail.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> Documentation/git-for-each-ref.txt | 4 ++++
> ref-filter.c | 25 +++++++++++++++++++------
> t/t6300-for-each-ref.sh | 10 ++++++++++
> 3 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index b7b8560..92184c4 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -107,6 +107,10 @@ objectsize::
> objectname::
> The object name (aka SHA-1).
> For a non-ambiguous abbreviation of the object name append `:short`.
> + For an abbreviation of the object name with desired length append
> + `:short=<length>`, where the minimum length is MINIMUM_ABBREV. The
> + length may be exceeded to ensure unique object names.
> +
>
> upstream::
> The name of a local ref which can be considered ``upstream''
> diff --git a/ref-filter.c b/ref-filter.c
> index 44481c3..fe4ea2b 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -55,7 +55,10 @@ static struct used_atom {
> const char *if_equals,
> *not_equals;
> } if_then_else;
> - enum { O_FULL, O_SHORT } objectname;
> + struct {
> + enum { O_FULL, O_LENGTH, O_SHORT } option;
> + unsigned int length;
> + } objectname;
> } u;
> } *used_atom;
> static int used_atom_cnt, need_tagged, need_symref;
> @@ -118,10 +121,17 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg)
> static void objectname_atom_parser(struct used_atom *atom, const char *arg)
> {
> if (!arg)
> - atom->u.objectname = O_FULL;
> + atom->u.objectname.option = O_FULL;
> else if (!strcmp(arg, "short"))
> - atom->u.objectname = O_SHORT;
> - else
> + atom->u.objectname.option = O_SHORT;
> + else if (skip_prefix(arg, "short=", &arg)) {
> + atom->u.objectname.option = O_LENGTH;
> + if (strtoul_ui(arg, 10, &atom->u.objectname.length) ||
> + atom->u.objectname.length == 0)
> + die(_("positive value expected objectname:short=%s"), arg);
> + if (atom->u.objectname.length < MINIMUM_ABBREV)
> + atom->u.objectname.length = MINIMUM_ABBREV;
One way to reduce some code is to set O_SHORT and O_LENGTH as the same
(either O_SHORT or O_LENGTH) and when no length is found simply set it
to the DEFAULT_ABBREV.
> + } else
> die(_("unrecognized %%(objectname) argument: %s"), arg);
> }
>
> @@ -591,12 +601,15 @@ static int grab_objectname(const char *name, const unsigned char *sha1,
> struct atom_value *v, struct used_atom *atom)
> {
> if (starts_with(name, "objectname")) {
> - if (atom->u.objectname == O_SHORT) {
> + if (atom->u.objectname.option == O_SHORT) {
> v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
> return 1;
That would allow dropping an entire section here.
I don't think this is worth a re-roll by itself, and I think either
approach is probably ok.
> - } else if (atom->u.objectname == O_FULL) {
> + } else if (atom->u.objectname.option == O_FULL) {
> v->s = xstrdup(sha1_to_hex(sha1));
> return 1;
> + } else if (atom->u.objectname.option == O_LENGTH) {
> + v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length));
> + return 1;
> } else
> die("BUG: unknown %%(objectname) option");
> }
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 19a2823..2be0a3f 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -60,6 +60,8 @@ test_atom head objecttype commit
> test_atom head objectsize 171
> test_atom head objectname $(git rev-parse refs/heads/master)
> test_atom head objectname:short $(git rev-parse --short refs/heads/master)
> +test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master)
> +test_atom head objectname:short=10 $(git rev-parse --short=10 refs/heads/master)
> test_atom head tree $(git rev-parse refs/heads/master^{tree})
> test_atom head parent ''
> test_atom head numparent 0
> @@ -99,6 +101,8 @@ test_atom tag objecttype tag
> test_atom tag objectsize 154
> test_atom tag objectname $(git rev-parse refs/tags/testtag)
> test_atom tag objectname:short $(git rev-parse --short refs/tags/testtag)
> +test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master)
> +test_atom head objectname:short=10 $(git rev-parse --short=10 refs/heads/master)
> test_atom tag tree ''
> test_atom tag parent ''
> test_atom tag numparent ''
> @@ -164,6 +168,12 @@ test_expect_success 'Check invalid format specifiers are errors' '
> test_must_fail git for-each-ref --format="%(authordate:INVALID)" refs/heads
> '
>
> +test_expect_success 'arguments to %(objectname:short=) must be positive integers' '
> + test_must_fail git for-each-ref --format="%(objectname:short=0)" &&
> + test_must_fail git for-each-ref --format="%(objectname:short=-1)" &&
> + test_must_fail git for-each-ref --format="%(objectname:short=foo)"
> +'
> +
> test_date () {
> f=$1 &&
> committer_date=$2 &&
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
From: Jacob Keller @ 2016-11-08 23:22 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-4-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Implement %(if:equals=<string>) wherein the if condition is only
> satisfied if the value obtained between the %(if:...) and %(then) atom
> is the same as the given '<string>'.
>
> Similarly, implement (if:notequals=<string>) wherein the if condition
> is only satisfied if the value obtained between the %(if:...) and
> %(then) atom is differnt from the given '<string>'.
>
> This is done by introducing 'if_atom_parser()' which parses the given
> %(if) atom and then stores the data in used_atom which is later passed
> on to the used_atom of the %(then) atom, so that it can do the required
> comparisons.
>
Ok. How does this handle whitespace? The previous if implementation
treated whitespace as trimming to ignore. Does this require an exact
whitespace match? It appears by the code that strings must match
exactly. Would it make more sense to always trim the value of
whitespace first before comparison? Hmm.. I think we should avoid
doing that actually.
Otherwise this looks good. I might have implemented the storage as one
value and then a boolean indicating whether to check for equality or
inequality. But I think the current implementation is ok too, and is a
bit more elegant on the code savings.
Thanks,
Jake
> Add tests and Documentation for the same.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> Documentation/git-for-each-ref.txt | 3 +++
> ref-filter.c | 43 +++++++++++++++++++++++++++++++++-----
> t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++
> 3 files changed, 59 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index fed8126..b7b8560 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -155,6 +155,9 @@ if::
> evaluating the string before %(then), this is useful when we
> use the %(HEAD) atom which prints either "*" or " " and we
> want to apply the 'if' condition only on the 'HEAD' ref.
> + Append ":equals=<string>" or ":notequals=<string>" to compare
> + the value between the %(if:...) and %(then) atoms with the
> + given string.
>
This seems to imply that it does not ignore whitespace. Ok.
> In addition to the above, for commit and tag objects, the header
> field names (`tree`, `parent`, `object`, `type`, and `tag`) can
> diff --git a/ref-filter.c b/ref-filter.c
> index 8392303..44481c3 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -22,6 +22,8 @@ struct align {
> };
>
> struct if_then_else {
> + const char *if_equals,
> + *not_equals;
Ok so we add both if_equals and not_equals values. Could we re-use the
same string?
> unsigned int then_atom_seen : 1,
> else_atom_seen : 1,
> condition_satisfied : 1;
> @@ -49,6 +51,10 @@ static struct used_atom {
> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
> unsigned int nlines;
> } contents;
> + struct {
> + const char *if_equals,
> + *not_equals;
Same here, why do we need both strings here stored separately? Could
we instead store which state to check and store the string once? I'm
not sure that really buys us any storage.
> + } if_then_else;
> enum { O_FULL, O_SHORT } objectname;
> } u;
> } *used_atom;
> @@ -169,6 +175,19 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)
> string_list_clear(¶ms, 0);
> }
>
> +static void if_atom_parser(struct used_atom *atom, const char *arg)
> +{
> + if (!arg)
> + return;
> + else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.if_equals))
> + ;
> + else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.not_equals))
> + ;
Ok so we can't ever have if_equals or not_equals at the same time.
> + else
> + die(_("unrecognized %%(if) argument: %s"), arg);
> +}
> +
> +
> static struct {
> const char *name;
> cmp_type cmp_type;
> @@ -209,7 +228,7 @@ static struct {
> { "color", FIELD_STR, color_atom_parser },
> { "align", FIELD_STR, align_atom_parser },
> { "end" },
> - { "if" },
> + { "if", FIELD_STR, if_atom_parser },
> { "then" },
> { "else" },
> };
> @@ -410,6 +429,9 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat
> struct ref_formatting_stack *new;
> struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
>
> + if_then_else->if_equals = atomv->atom->u.if_then_else.if_equals;
> + if_then_else->not_equals = atomv->atom->u.if_then_else.not_equals;
> +
> push_stack_element(&state->stack);
> new = state->stack;
> new->at_end = if_then_else_handler;
> @@ -441,10 +463,17 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
> die(_("format: %%(then) atom used after %%(else)"));
> if_then_else->then_atom_seen = 1;
> /*
> - * If there exists non-empty string between the 'if' and
> - * 'then' atom then the 'if' condition is satisfied.
> + * If the 'equals' or 'notequals' attribute is used then
> + * perform the required comparison. If not, only non-empty
> + * strings satisfy the 'if' condition.
> */
> - if (cur->output.len && !is_empty(cur->output.buf))
> + if (if_then_else->if_equals) {
> + if (!strcmp(if_then_else->if_equals, cur->output.buf))
> + if_then_else->condition_satisfied = 1;
Ok so if somehow we end up with both set, if_equals takes precedence.
I think I can be ok with the way we handle this.
> + } else if (if_then_else->not_equals) {
> + if (strcmp(if_then_else->not_equals, cur->output.buf))
> + if_then_else->condition_satisfied = 1;
> + } else if (cur->output.len && !is_empty(cur->output.buf))
> if_then_else->condition_satisfied = 1;
> strbuf_reset(&cur->output);
> }
> @@ -1137,7 +1166,11 @@ static void populate_value(struct ref_array_item *ref)
> } else if (!strcmp(name, "end")) {
> v->handler = end_atom_handler;
> continue;
> - } else if (!strcmp(name, "if")) {
> + } else if (starts_with(name, "if")) {
> + const char *s;
> +
> + if (skip_prefix(name, "if:", &s))
> + v->s = xstrdup(s);
> v->handler = if_atom_handler;
> continue;
> } else if (!strcmp(name, "then")) {
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index fed3013..a09a1a4 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -403,4 +403,22 @@ test_expect_success 'ignore spaces in %(if) atom usage' '
> test_cmp expect actual
> '
>
> +test_expect_success 'check %(if:equals=<string>)' '
> + git for-each-ref --format="%(if:equals=master)%(refname:short)%(then)Found master%(else)Not master%(end)" refs/heads/ >actual &&
> + cat >expect <<-\EOF &&
> + Found master
> + Not master
> + EOF
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'check %(if:notequals=<string>)' '
> + git for-each-ref --format="%(if:notequals=master)%(refname:short)%(then)Not master%(else)Found master%(end)" refs/heads/ >actual &&
> + cat >expect <<-\EOF &&
> + Found master
> + Not master
> + EOF
> + test_cmp expect actual
> +'
> +
> test_done
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 02/17] ref-filter: include reference to 'used_atom' within 'atom_value'
From: Jacob Keller @ 2016-11-08 23:16 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-3-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Ensure that each 'atom_value' has a reference to its corresponding
> 'used_atom'. This let's us use values within 'used_atom' in the
> 'handler' function.
>
> Hence we can get the %(align) atom's parameters directly from the
> 'used_atom' therefore removing the necessity of passing %(align) atom's
> parameters to 'atom_value'.
>
> This also acts as a preparatory patch for the upcoming patch where we
> introduce %(if:equals=) and %(if:notequals=).
>
Makes sense.
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> ref-filter.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 8c183a0..8392303 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -230,11 +230,9 @@ struct ref_formatting_state {
>
> struct atom_value {
> const char *s;
> - union {
> - struct align align;
> - } u;
> void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
> unsigned long ul; /* used for sorting when not FIELD_STR */
> + struct used_atom *atom;
> };
>
> /*
> @@ -370,7 +368,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
> push_stack_element(&state->stack);
> new = state->stack;
> new->at_end = end_align_handler;
> - new->at_end_data = &atomv->u.align;
> + new->at_end_data = &atomv->atom->u.align;
At first, this confused me. I was like "we dropped the union, why are
we still referencing it. But I realized that the "used_atom" struct
actually contains the same union and we were copying it.
Ok, so this looks good.
Thanks,
Jake
> }
>
> static void if_then_else_handler(struct ref_formatting_stack **stack)
> @@ -1069,6 +1067,7 @@ static void populate_value(struct ref_array_item *ref)
> struct branch *branch = NULL;
>
> v->handler = append_atom;
> + v->atom = atom;
>
> if (*name == '*') {
> deref = 1;
> @@ -1133,7 +1132,6 @@ static void populate_value(struct ref_array_item *ref)
> v->s = " ";
> continue;
> } else if (starts_with(name, "align")) {
> - v->u.align = atom->u.align;
> v->handler = align_atom_handler;
> continue;
> } else if (!strcmp(name, "end")) {
> --
> 2.10.2
>
^ permalink raw reply
* Re: [PATCH v7 01/17] ref-filter: implement %(if), %(then), and %(else) atoms
From: Jacob Keller @ 2016-11-08 23:13 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <20161108201211.25213-2-Karthik.188@gmail.com>
On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Implement %(if), %(then) and %(else) atoms. Used as
> %(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the
> format string between %(if) and %(then) expands to an empty string, or
> to only whitespaces, then the whole %(if)...%(end) expands to the string
> following %(then). Otherwise, it expands to the string following
> %(else), if any. Nesting of this construct is possible.
>
> This is in preparation for porting over `git branch -l` to use
> ref-filter APIs for printing.
>
> Add Documentation and tests regarding the same.
>
Ok, so I have only one minor nit, but otherwise this looks quite good
to me. A few comments explaining my understanding, but only one
suggested
change which is really a minor nit and not worth re-rolling just for it.
Thanks,
Jake
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> Documentation/git-for-each-ref.txt | 40 +++++++++++
> ref-filter.c | 133 +++++++++++++++++++++++++++++++++++--
> t/t6302-for-each-ref-filter.sh | 76 +++++++++++++++++++++
> 3 files changed, 242 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index f57e69b..fed8126 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -146,6 +146,16 @@ align::
> quoted, but if nested then only the topmost level performs
> quoting.
>
> +if::
> + Used as %(if)...%(then)...(%end) or
> + %(if)...%(then)...%(else)...%(end). If there is an atom with
> + value or string literal after the %(if) then everything after
> + the %(then) is printed, else if the %(else) atom is used, then
> + everything after %(else) is printed. We ignore space when
> + evaluating the string before %(then), this is useful when we
> + use the %(HEAD) atom which prints either "*" or " " and we
> + want to apply the 'if' condition only on the 'HEAD' ref.
> +
> In addition to the above, for commit and tag objects, the header
> field names (`tree`, `parent`, `object`, `type`, and `tag`) can
> be used to specify the value in the header field.
> @@ -181,6 +191,20 @@ As a special case for the date-type fields, you may specify a format for
> the date by adding `:` followed by date format name (see the
> values the `--date` option to linkgit:git-rev-list[1] takes).
>
> +Some atoms like %(align) and %(if) always require a matching %(end).
> +We call them "opening atoms" and sometimes denote them as %($open).
> +
> +When a scripting language specific quoting is in effect (i.e. one of
> +`--shell`, `--perl`, `--python`, `--tcl` is used), except for opening
> +atoms, replacement from every %(atom) is quoted when and only when it
> +appears at the top-level (that is, when it appears outside
> +%($open)...%(end)).
> +
> +When a scripting language specific quoting is in effect, everything
> +between a top-level opening atom and its matching %(end) is evaluated
> +according to the semantics of the opening atom and its result is
> +quoted.
> +
>
Nice, I like the explanation above.
> EXAMPLES
> --------
> @@ -268,6 +292,22 @@ eval=`git for-each-ref --shell --format="$fmt" \
> eval "$eval"
> ------------
>
> +
> +An example to show the usage of %(if)...%(then)...%(else)...%(end).
> +This prefixes the current branch with a star.
> +
> +------------
> +git for-each-ref --format="%(if)%(HEAD)%(then)* %(else) %(end)%(refname:short)" refs/heads/
> +------------
> +
> +
> +An example to show the usage of %(if)...%(then)...%(end).
> +This prints the authorname, if present.
> +
> +------------
> +git for-each-ref --format="%(refname)%(if)%(authorname)%(then) %(color:red)Authored by: %(authorname)%(end)"
> +------------
> +
> SEE ALSO
> --------
> linkgit:git-show-ref[1]
> diff --git a/ref-filter.c b/ref-filter.c
> index d4c2931..8c183a0 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -21,6 +21,12 @@ struct align {
> unsigned int width;
> };
>
> +struct if_then_else {
> + unsigned int then_atom_seen : 1,
> + else_atom_seen : 1,
> + condition_satisfied : 1;
> +};
> +
> /*
> * An atom is a valid field atom listed below, possibly prefixed with
> * a "*" to denote deref_tag().
> @@ -203,6 +209,9 @@ static struct {
> { "color", FIELD_STR, color_atom_parser },
> { "align", FIELD_STR, align_atom_parser },
> { "end" },
> + { "if" },
> + { "then" },
> + { "else" },
> };
>
> #define REF_FORMATTING_STATE_INIT { 0, NULL }
> @@ -210,7 +219,7 @@ static struct {
> struct ref_formatting_stack {
> struct ref_formatting_stack *prev;
> struct strbuf output;
> - void (*at_end)(struct ref_formatting_stack *stack);
> + void (*at_end)(struct ref_formatting_stack **stack);
> void *at_end_data;
> };
>
> @@ -343,13 +352,14 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
> *stack = prev;
> }
>
> -static void end_align_handler(struct ref_formatting_stack *stack)
> +static void end_align_handler(struct ref_formatting_stack **stack)
> {
So we now have to pass an array of stacks to the end_align_handler
instead? Ok. But for align this is simple since it just expects a
singleton.
> - struct align *align = (struct align *)stack->at_end_data;
> + struct ref_formatting_stack *cur = *stack;
> + struct align *align = (struct align *)cur->at_end_data;
> struct strbuf s = STRBUF_INIT;
>
> - strbuf_utf8_align(&s, align->position, align->width, stack->output.buf);
> - strbuf_swap(&stack->output, &s);
> + strbuf_utf8_align(&s, align->position, align->width, cur->output.buf);
> + strbuf_swap(&cur->output, &s);
> strbuf_release(&s);
> }
>
> @@ -363,6 +373,103 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
> new->at_end_data = &atomv->u.align;
> }
>
> +static void if_then_else_handler(struct ref_formatting_stack **stack)
> +{
> + struct ref_formatting_stack *cur = *stack;
> + struct ref_formatting_stack *prev = cur->prev;
> + struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
> +
> + if (!if_then_else->then_atom_seen)
> + die(_("format: %%(if) atom used without a %%(then) atom"));
> +
> + if (if_then_else->else_atom_seen) {
> + /*
> + * There is an %(else) atom: we need to drop one state from the
> + * stack, either the %(else) branch if the condition is satisfied, or
> + * the %(then) branch if it isn't.
> + */
> + if (if_then_else->condition_satisfied) {
> + strbuf_reset(&cur->output);
> + pop_stack_element(&cur);
So here, once we have a satisfied condition, we just drop the "else"
element entirely.
> + } else {
> + strbuf_swap(&cur->output, &prev->output);
> + strbuf_reset(&cur->output);
> + pop_stack_element(&cur);
Otherwise, we swap our current value into the value of the previous
element, and then drop the current. This is a bit tricky, but it
works.
> + }
> + } else if (!if_then_else->condition_satisfied)
Minor nit. I'm not sure what standard we use here at Git, but
traditionally, I prefer to see { } blocks on all sections even if only
one of them needs it. (That is, only drop the braces when every
section is one line.) It also looks weird with a comment since it
appears as multiple lines to the reader. I think the braces improve
readability.
I don't know whether that's Git's code base standard or not, however.
It's not really worth a re-roll unless something else would need to
change.
> + /*
> + * No %(else) atom: just drop the %(then) branch if the
> + * condition is not satisfied.
> + */
> + strbuf_reset(&cur->output);
Finally, if no else element, then we just reset the current pointer.
> +
> + *stack = cur;
> + free(if_then_else);
> +}
> +
> +static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> +{
> + struct ref_formatting_stack *new;
> + struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
> +
> + push_stack_element(&state->stack);
> + new = state->stack;
> + new->at_end = if_then_else_handler;
> + new->at_end_data = if_then_else;
> +}
> +
Ok, so the new method is that to handle "if"s we push the sets onto
the stack and check their values. I like this, it makes things pretty
straight forward and simple. Allows for quite a bit of expression.
> +static int is_empty(const char *s)
> +{
> + while (*s != '\0') {
> + if (!isspace(*s))
> + return 0;
> + s++;
> + }
> + return 1;
> +}
> +
> +static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> +{
> + struct ref_formatting_stack *cur = state->stack;
> + struct if_then_else *if_then_else = NULL;
> +
> + if (cur->at_end == if_then_else_handler)
> + if_then_else = (struct if_then_else *)cur->at_end_data;
> + if (!if_then_else)
> + die(_("format: %%(then) atom used without an %%(if) atom"));
> + if (if_then_else->then_atom_seen)
> + die(_("format: %%(then) atom used more than once"));
> + if (if_then_else->else_atom_seen)
> + die(_("format: %%(then) atom used after %%(else)"));
> + if_then_else->then_atom_seen = 1;
> + /*
> + * If there exists non-empty string between the 'if' and
> + * 'then' atom then the 'if' condition is satisfied.
> + */
> + if (cur->output.len && !is_empty(cur->output.buf))
> + if_then_else->condition_satisfied = 1;
> + strbuf_reset(&cur->output);
> +}
So once we have a "%(then)" atom, we reset all the accumulated string
data we've gotten so far. Simple.
> +
> +static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> +{
> + struct ref_formatting_stack *prev = state->stack;
> + struct if_then_else *if_then_else = NULL;
> +
> + if (prev->at_end == if_then_else_handler)
> + if_then_else = (struct if_then_else *)prev->at_end_data;
> + if (!if_then_else)
> + die(_("format: %%(else) atom used without an %%(if) atom"));
> + if (!if_then_else->then_atom_seen)
> + die(_("format: %%(else) atom used without a %%(then) atom"));
> + if (if_then_else->else_atom_seen)
> + die(_("format: %%(else) atom used more than once"));
> + if_then_else->else_atom_seen = 1;
> + push_stack_element(&state->stack);
> + state->stack->at_end_data = prev->at_end_data;
> + state->stack->at_end = prev->at_end;
> +}
So for an else atom, we basically create another stack element on top
of the current one. Nice.
> +
> static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> {
> struct ref_formatting_stack *current = state->stack;
> @@ -370,14 +477,17 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
>
> if (!current->at_end)
> die(_("format: %%(end) atom used without corresponding atom"));
> - current->at_end(current);
> + current->at_end(&state->stack);
> +
> + /* Stack may have been popped within at_end(), hence reset the current pointer */
> + current = state->stack;
>
> /*
> * Perform quote formatting when the stack element is that of
> * a supporting atom. If nested then perform quote formatting
> * only on the topmost supporting atom.
> */
> - if (!state->stack->prev->prev) {
> + if (!current->prev->prev) {
> quote_formatting(&s, current->output.buf, state->quote_style);
> strbuf_swap(¤t->output, &s);
> }
> @@ -1029,6 +1139,15 @@ static void populate_value(struct ref_array_item *ref)
> } else if (!strcmp(name, "end")) {
> v->handler = end_atom_handler;
> continue;
> + } else if (!strcmp(name, "if")) {
> + v->handler = if_atom_handler;
> + continue;
> + } else if (!strcmp(name, "then")) {
> + v->handler = then_atom_handler;
> + continue;
> + } else if (!strcmp(name, "else")) {
> + v->handler = else_atom_handler;
> + continue;
> } else
> continue;
>
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index d0ab09f..fed3013 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -327,4 +327,80 @@ test_expect_success 'reverse version sort' '
> test_cmp expect actual
> '
>
> +test_expect_success 'improper usage of %(if), %(then), %(else) and %(end) atoms' '
> + test_must_fail git for-each-ref --format="%(if)" &&
> + test_must_fail git for-each-ref --format="%(then) %(end)" &&
> + test_must_fail git for-each-ref --format="%(else) %(end)" &&
> + test_must_fail git for-each-ref --format="%(if) %(else) %(end)" &&
> + test_must_fail git for-each-ref --format="%(if) %(then) %(then) %(end)" &&
> + test_must_fail git for-each-ref --format="%(then) %(else) %(end)" &&
> + test_must_fail git for-each-ref --format="%(if) %(else) %(end)" &&
> + test_must_fail git for-each-ref --format="%(if) %(then) %(else)" &&
> + test_must_fail git for-each-ref --format="%(if) %(else) %(then) %(end)" &&
> + test_must_fail git for-each-ref --format="%(if) %(then) %(else) %(else) %(end)" &&
> + test_must_fail git for-each-ref --format="%(if) %(end)"
> +'
> +
> +test_expect_success 'check %(if)...%(then)...%(end) atoms' '
> + git for-each-ref --format="%(refname)%(if)%(authorname)%(then) Author: %(authorname)%(end)" >actual &&
> + cat >expect <<-\EOF &&
> + refs/heads/master Author: A U Thor
> + refs/heads/side Author: A U Thor
> + refs/odd/spot Author: A U Thor
> + refs/tags/annotated-tag
> + refs/tags/doubly-annotated-tag
> + refs/tags/doubly-signed-tag
> + refs/tags/foo1.10 Author: A U Thor
> + refs/tags/foo1.3 Author: A U Thor
> + refs/tags/foo1.6 Author: A U Thor
> + refs/tags/four Author: A U Thor
> + refs/tags/one Author: A U Thor
> + refs/tags/signed-tag
> + refs/tags/three Author: A U Thor
> + refs/tags/two Author: A U Thor
> + EOF
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'check %(if)...%(then)...%(else)...%(end) atoms' '
> + git for-each-ref --format="%(if)%(authorname)%(then)%(authorname)%(else)No author%(end): %(refname)" >actual &&
> + cat >expect <<-\EOF &&
> + A U Thor: refs/heads/master
> + A U Thor: refs/heads/side
> + A U Thor: refs/odd/spot
> + No author: refs/tags/annotated-tag
> + No author: refs/tags/doubly-annotated-tag
> + No author: refs/tags/doubly-signed-tag
> + A U Thor: refs/tags/foo1.10
> + A U Thor: refs/tags/foo1.3
> + A U Thor: refs/tags/foo1.6
> + A U Thor: refs/tags/four
> + A U Thor: refs/tags/one
> + No author: refs/tags/signed-tag
> + A U Thor: refs/tags/three
> + A U Thor: refs/tags/two
> + EOF
> + test_cmp expect actual
> +'
> +test_expect_success 'ignore spaces in %(if) atom usage' '
> + git for-each-ref --format="%(refname:short): %(if)%(HEAD)%(then)Head ref%(else)Not Head ref%(end)" >actual &&
> + cat >expect <<-\EOF &&
> + master: Head ref
> + side: Not Head ref
> + odd/spot: Not Head ref
> + annotated-tag: Not Head ref
> + doubly-annotated-tag: Not Head ref
> + doubly-signed-tag: Not Head ref
> + foo1.10: Not Head ref
> + foo1.3: Not Head ref
> + foo1.6: Not Head ref
> + four: Not Head ref
> + one: Not Head ref
> + signed-tag: Not Head ref
> + three: Not Head ref
> + two: Not Head ref
> + EOF
> + test_cmp expect actual
> +'
> +
> test_done
> --
> 2.10.2
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox