* Re: Why are there multiple ways to get the manual in Git?
From: Philip Oakley @ 2016-09-17 19:12 UTC (permalink / raw)
To: Fredrik Gustafsson, Andrew Johnson; +Cc: git
In-Reply-To: <20160917183919.GJ20666@paksenarrion.iveqy.com>
From: "Fredrik Gustafsson" <iveqy@iveqy.com>
> On Sat, Sep 17, 2016 at 01:47:52PM -0400, Andrew Johnson wrote:
>> $ git help <verb>
>> $ git <verb> --help
>> $ man git-<verb>
>>
>> I tested all three to confirm they were equivalent.
It is (IIUC) in a general sort of way "by design", and a little bit of
accident.
>
> While I'm not able to answer your question, I can shred a little light
> about them not being equal. For example using a windows machine
>
> $ man git <verb>
>
> does not work and
>
> $ git help <verb>
>
> opens a webbrowser instead of a man page. Using a unix system I would
> however assume that
>
> $ man git <verb>
>
> would work since it's the standard way of getting help on those systems.
>
> --
Historically git was a set of shell scripts named git-*, so each stood
alone.
Then there was the great consolidation (around V1.6?) which created the
modern `git <cmd>' approach, with every command normally having -h
and --help options for short form usage and long form man pages.
The option capability became standardised. Also a `git help <cmd>` command
was created. Underneath there are still the (backward compatible) git-*
forms. The help command allowed selection of display type, so that on
Unix/Linux man was the norm, while an --html (or --web) option is available
for those who like the pretty browser view
The help commnad just converts the parameters to achieve the expected
display (with various fallbacks if the command or guide is missing, etc)
Meanwhile on Windows, the man facility was not ported as part of git, so it
defaults to the --web version. If you are on Windows, and download the SDK
as well you can install the man viewer and other goodies
--
Philip
^ permalink raw reply
* Re: [PATCH v2] format-patch: Add --rfc for the common case of [RFC PATCH]
From: Jeff King @ 2016-09-17 18:43 UTC (permalink / raw)
To: Josh Triplett; +Cc: git, Andrew Donnellan
In-Reply-To: <b5bf39015fdd20dd0aa4f38eb365bbbd0d07a4ca.1474096535.git-series.josh@joshtriplett.org>
On Sat, Sep 17, 2016 at 12:21:52AM -0700, Josh Triplett wrote:
> This provides a shorter and more convenient alias for
> --subject-prefix='RFC PATCH'.
>
> Includes documentation in the format-patch manpage, and a new test
> covering --rfc.
>
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
> v2:
> - Add documentation to the format-patch manpage
> - Call subject_prefix_callback rather than reimplementing it
> - Update test to move expectations inside
Assuming we want this option, the implementation looks good to me (and I
don't have a big opinion the first part of that sentence).
-Peff
^ permalink raw reply
* Re: Why are there multiple ways to get the manual in Git?
From: Fredrik Gustafsson @ 2016-09-17 18:39 UTC (permalink / raw)
To: Andrew Johnson; +Cc: git
In-Reply-To: <CAM_5GX48gDAZSvAWnxO5n8uhYf8vmfAJ88_31_ewsQxyPfF7iA@mail.gmail.com>
On Sat, Sep 17, 2016 at 01:47:52PM -0400, Andrew Johnson wrote:
> $ git help <verb>
> $ git <verb> --help
> $ man git-<verb>
>
> I tested all three to confirm they were equivalent.
While I'm not able to answer your question, I can shred a little light
about them not being equal. For example using a windows machine
$ man git <verb>
does not work and
$ git help <verb>
opens a webbrowser instead of a man page. Using a unix system I would
however assume that
$ man git <verb>
would work since it's the standard way of getting help on those systems.
--
Fredrik Gustafsson
phone: +46 733-608274
e-mail: iveqy@iveqy.com
website: http://www.iveqy.com
^ permalink raw reply
* Re: Two bugs in --pretty with %C(auto)
From: René Scharfe @ 2016-09-17 18:25 UTC (permalink / raw)
To: Anatoly Borodin, Duy Nguyen; +Cc: git, Junio C Hamano
In-Reply-To: <nrje96$q7s$1@blaine.gmane.org>
Am 17.09.2016 um 14:51 schrieb Anatoly Borodin:
> Hi All!
>
> First bug:
>
> git log -3 --pretty='%C(cyan)%C(auto)%h%C(auto)%d %s'
>
> prints %h with the default color (normal yellow), but
>
> git log -3 --pretty='%C(bold cyan)%C(auto)%h%C(auto)%d %s'
>
> shows %h with bold yellow, as if only the color was reset, but not
> the attributes (blink, ul, reverse also work this way). %d and %s are
> printed with the right color both times.
>
> Second bug, maybe related to the first one:
>
> git log -3 --pretty='%C(bold cyan)%h%C(auto)%d %s %an %h %h %s'
>
> The first line looks as expected. Well, almost: the '(' of %d is bold
> yellow.
>
> The second line looks like this:
>
> * %h, %s, %an with bold cyan;
> * %h with bold yellow;
> * %h with normal yellow and %s with normal white (default colors).
>
> PS git version 2.9.2
Well, in both cases you could add %Creset before %C(auto) to get what
you want.
I'm not sure how just how automatic %C(auto) is supposed to be, but you
expected it do emit the reset for you, right? Sounds reasonable to me.
The following patch implements that behavior.
Duy, what do you think?
-- >8 --
Subject: pretty: let %C(auto) reset all attributes
Reset colors and attributes upon %C(auto) to enable full automatic
control over them; otherwise attributes like bold or reverse could
still be in effect from previous %C placeholders.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
pretty.c | 2 ++
t/t6006-rev-list-format.sh | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/pretty.c b/pretty.c
index 9788bd8..493edb0 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1072,6 +1072,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
case 'C':
if (starts_with(placeholder + 1, "(auto)")) {
c->auto_color = want_color(c->pretty_ctx->color);
+ if (c->auto_color)
+ strbuf_addstr(sb, GIT_COLOR_RESET);
return 7; /* consumed 7 bytes, "C(auto)" */
} else {
int ret = parse_color(sb, placeholder, c);
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index a1dcdb8..f6020cd 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -225,7 +225,7 @@ test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '
test_expect_success '%C(auto) respects --color' '
git log --color --format="%C(auto)%H" -1 >actual &&
- printf "\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect &&
+ printf "\\033[m\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect &&
test_cmp expect actual
'
--
2.10.0
^ permalink raw reply related
* Why are there multiple ways to get the manual in Git?
From: Andrew Johnson @ 2016-09-17 17:47 UTC (permalink / raw)
To: git
Hi,
I was curious as to why the developers of Git decided to have three
methods to get the manual for a verb. I am a developer who strives to
understand Git to its fullest extent, and will share any information
given on this question.
While reading Pro Git 2nd Ed. I came across these three methods:
$ git help <verb>
$ git <verb> --help
$ man git-<verb>
I tested all three to confirm they were equivalent.
What was the motivation behind the complication, if any? I presume
most developers would not provide multiple commands that do the same
thing for absolutely no reason, so I led myself to ask this question.
Respectfully,
Andrew Johnson
^ permalink raw reply
* Re: [wishlist] disable boring messages
From: Anatoly Borodin @ 2016-09-17 16:03 UTC (permalink / raw)
To: git
In-Reply-To: <20160916221753.pvqdwb7vspkosyxu@shurick.grid.su>
Hi!
Alexander Inyukhin <shurick@sectorb.msk.ru> wrote:
> I have a lot of repos and a batch script to update them all,
> and I want to get rid of 'Fetching origin' and 'Already up-to-date.'
> messages leaving only new refs and tags.
There is an option `-q`, but it's too silent :)
As far as I can see, `git fetch` prints 'Fetching origin' etc to stdout,
and new refs/tags to stderr (don't ask me why). So
git fetch blabla > /dev/null
should probably do the job.
Now, if you collect and save those logs, you may need some shell-fu to
redirect that stderr to stdout, but not to /dev/null. Something like
(git fetch blabla > /dev/null) 2>&1
(run `git fetch` in a subshell)...
--
Mit freundlichen Grüßen,
Anatoly Borodin
^ permalink raw reply
* Finding a commit based on the diff index line?
From: Philip Oakley @ 2016-09-17 14:33 UTC (permalink / raw)
To: Git List
Hi,
I'm curating some of my old patch series (i.e. doing some tidying up) and
I'm trying to determine the commits that generated some of my patches so
that I can see if I still have them after they were rebased (a 'name that
dangling branch' problem).
Is there an easy way of finding the commit sha1 that contains the given diff
index line.
For example.
index fa05269..57033dd 100755
or
index 8ebcded..d9ab360 100644
which both should get back to Jeff King's 36d6792 (t0006: test various date
formats, 2016-06-20).
It feels like it is something that should already possible without a
mini-script. We have the rev range which should limit the range to a single
commit, though if random blob revs were given the commit range would be
'scattered'.
Is there a simple quick way of achieving this?
--
Philip
^ permalink raw reply
* Two bugs in --pretty with %C(auto)
From: Anatoly Borodin @ 2016-09-17 12:51 UTC (permalink / raw)
To: git
Hi All!
First bug:
git log -3 --pretty='%C(cyan)%C(auto)%h%C(auto)%d %s'
prints %h with the default color (normal yellow), but
git log -3 --pretty='%C(bold cyan)%C(auto)%h%C(auto)%d %s'
shows %h with bold yellow, as if only the color was reset, but not
the attributes (blink, ul, reverse also work this way). %d and %s are
printed with the right color both times.
Second bug, maybe related to the first one:
git log -3 --pretty='%C(bold cyan)%h%C(auto)%d %s %an %h %h %s'
The first line looks as expected. Well, almost: the '(' of %d is bold
yellow.
The second line looks like this:
* %h, %s, %an with bold cyan;
* %h with bold yellow;
* %h with normal yellow and %s with normal white (default colors).
PS git version 2.9.2
--
Mit freundlichen Grüßen,
Anatoly Borodin
^ permalink raw reply
* Re: Potentially misleading color.* defaults explanation in git-config(1)
From: Anatoly Borodin @ 2016-09-17 8:16 UTC (permalink / raw)
To: git
In-Reply-To: <vpqeg4k1f3g.fsf@anie.imag.fr>
Hi!
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> My bad, I forgot to update these parts of the docs when changing the
> default for color.ui (a while back already). Patch follows.
Thanks for the patch!
> git -c color.branch=false git branch
Oh, that's a nice one! I don't get a chance to use those
between-git-and-command options often.
Merci!
--
Mit freundlichen Grüßen,
Anatoly Borodin
^ permalink raw reply
* [PATCH v2] format-patch: Add --rfc for the common case of [RFC PATCH]
From: Josh Triplett @ 2016-09-17 7:21 UTC (permalink / raw)
To: git; +Cc: Jeff King, Andrew Donnellan
This provides a shorter and more convenient alias for
--subject-prefix='RFC PATCH'.
Includes documentation in the format-patch manpage, and a new test
covering --rfc.
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
v2:
- Add documentation to the format-patch manpage
- Call subject_prefix_callback rather than reimplementing it
- Update test to move expectations inside
Documentation/git-format-patch.txt | 8 +++++++-
builtin/log.c | 8 ++++++++
t/t4014-format-patch.sh | 9 +++++++++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 9624c84..b9590a5 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -19,7 +19,8 @@ SYNOPSIS
[--start-number <n>] [--numbered-files]
[--in-reply-to=Message-Id] [--suffix=.<sfx>]
[--ignore-if-in-upstream]
- [--subject-prefix=Subject-Prefix] [(--reroll-count|-v) <n>]
+ [--rfc] [--subject-prefix=Subject-Prefix]
+ [(--reroll-count|-v) <n>]
[--to=<email>] [--cc=<email>]
[--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
[<common diff options>]
@@ -172,6 +173,11 @@ will want to ensure that threading is disabled for `git send-email`.
allows for useful naming of a patch series, and can be
combined with the `--numbered` option.
+--rfc::
+ Alias for `--subject-prefix="RFC PATCH"`. Use this when
+ sending an experimental patch for discussion rather than
+ application.
+
-v <n>::
--reroll-count=<n>::
Mark the series as the <n>-th iteration of the topic. The
diff --git a/builtin/log.c b/builtin/log.c
index 92dc34d..5757d91 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1112,6 +1112,11 @@ static int subject_prefix_callback(const struct option *opt, const char *arg,
return 0;
}
+static int rfc_callback(const struct option *opt, const char *arg, int unset)
+{
+ return subject_prefix_callback(opt, "RFC PATCH", unset);
+}
+
static int numbered_cmdline_opt = 0;
static int numbered_callback(const struct option *opt, const char *arg,
@@ -1419,6 +1424,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
N_("start numbering patches at <n> instead of 1")),
OPT_INTEGER('v', "reroll-count", &reroll_count,
N_("mark the series as Nth re-roll")),
+ { OPTION_CALLBACK, 0, "rfc", &rev, NULL,
+ N_("Use [RFC PATCH] instead of [PATCH]"),
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG, rfc_callback },
{ OPTION_CALLBACK, 0, "subject-prefix", &rev, N_("prefix"),
N_("Use [<prefix>] instead of [PATCH]"),
PARSE_OPT_NONEG, subject_prefix_callback },
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index b0579dd..ed4d3c2 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1073,6 +1073,15 @@ test_expect_success 'empty subject prefix does not have extra space' '
test_cmp expect actual
'
+test_expect_success '--rfc' '
+ cat >expect <<-\EOF &&
+ Subject: [RFC PATCH 1/1] header with . in it
+ EOF
+ git format-patch -n -1 --stdout --rfc >patch &&
+ grep ^Subject: patch >actual &&
+ test_cmp expect actual
+'
+
test_expect_success '--from=ident notices bogus ident' '
test_must_fail git format-patch -1 --stdout --from=foo >patch
'
base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b
--
git-series 0.8.10
^ permalink raw reply related
* Re: [PATCH] format-patch: Add --rfc for the common case of [RFC PATCH]
From: Josh Triplett @ 2016-09-17 4:20 UTC (permalink / raw)
To: Jeff King; +Cc: git, Andrew Donnellan
In-Reply-To: <20160917004922.rd6g3bqajyt4iyjm@sigill.intra.peff.net>
On Fri, Sep 16, 2016 at 05:49:22PM -0700, Jeff King wrote:
> On Fri, Sep 16, 2016 at 10:27:45AM -0700, Josh Triplett wrote:
>
> > By far, the most common subject-prefix I've seen other than "PATCH" is
> > "RFC PATCH" (or occasionally "PATCH RFC"). Seems worth optimizing for
> > the common case, to avoid having to spell it out the long way as
> > --subject-prefix='RFC PATCH'.
>
> "RFC" is the most common one for me, too. And if it ends here, I'm OK
> with it. But I'm a little worried with ending up with a proliferation of
> options.
I haven't seen a significant number of variations on subject prefixes; I
can't think of any other prefix I've seen often enough to suggest an
option.
> If we had a short-option for --subject-prefix, then:
>
> -P RFC
>
> is not so bad compared to "--rfc". But if you want to spell it as "RFC
> PATCH" that's getting a bit longer. We could have a short option for
> "tag this in the subject prefix _in addition_ to writing PATCH". And
> then you could do:
>
> -T RFC
>
> I dunno. One other thing to consider is that format-patch takes
> arbitrary diff options, so we'd want to avoid stomping on them with any
> short options (which is why I used "-T" instead of "-t", though I find
> it unlikely that many people use the latter with format-patch). That's a
> point in favor of --rfc, I think.
I agree; the short option space seems more contentious. And in any
case, I find --rfc more ergonomic than "-T RFC". :)
> > builtin/log.c | 10 ++++++++++
> > t/t4014-format-patch.sh | 9 +++++++++
> > 2 files changed, 19 insertions(+), 0 deletions(-)
>
> Documentation?
Oops, thanks. I'll send v2 shortly.
> > +static int rfc_callback(const struct option *opt, const char *arg, int unset)
> > +{
> > + subject_prefix = 1;
> > + ((struct rev_info *)opt->value)->subject_prefix = xstrdup("RFC PATCH");
> > + return 0;
> > +}
>
> I was going to complain that you don't free() the previous value, but
> actually the other callers do not xstrdup() in the first place (and we
> do not need to do so here, either, as it's a string literal). We
> actually _do_ allocate a new copy when reading the value from config,
> but it's probably not a big deal in practice to leak that.
>
> I also wonder if you could implement this as just:
>
> return subject_prefix_callback(opt, "RFC PATCH", unset);
>
> And then if you write the documentation as:
>
> --rfc::
> Behave as if --subject-prefix="RFC PATCH" was specified.
>
> then it will be trivially correct. :)
Nice idea; will do.
> > +cat >expect <<'EOF'
> > +Subject: [RFC PATCH 1/1] header with . in it
> > +EOF
> > +test_expect_success '--rfc' '
> > + git format-patch -n -1 --stdout --rfc >patch &&
> > + grep ^Subject: patch >actual &&
> > + test_cmp expect actual
> > +'
>
> Our usual style these days is to set up expectations inside the test
> blocks (and use "<<-" to get nice indentation; we also typically use
> "\EOF" but that's purely style).
I copied this from a test immediately above it. :)
I can change it easily enough, though.
- Josh Triplett
^ permalink raw reply
* Re: [RFC/PATCH 2/3] mailinfo: correct malformed test example
From: Junio C Hamano @ 2016-09-17 3:48 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, peff
In-Reply-To: <3a27685f-a53b-03a7-93d5-0492638faf51@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> From: bogosity
> - a list
> - of stuff
>
> Unchanged, the subsequent patch would break this test because it would
> interpret that as a multi-line "From" in-body header when in-body
> headers are *not* disabled.
Yes, that is totally expected. So I would be perfectly fine if your
patch changed the test vector for that case, saying "Allowing a
folded in-body header means the expected result for the above three
lines has to change".
^ permalink raw reply
* Re: [PATCH] ls-files: add pathspec matching for submodules
From: Junio C Hamano @ 2016-09-17 3:46 UTC (permalink / raw)
To: Brandon Williams
Cc: git, Heiko Voigt, Nguyễn Thái Ngọc Duy,
Stefan Beller
In-Reply-To: <1474073981-96620-1-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> ...
> [--full-name] [--recurse-submodules]
> - [--output-path-prefix=<path>]
> + [--submodule-prefix=<path>]
> [--abbrev] [--] [<file>...]
>
> ---output-path-prefix=<path>::
> +--submodule-prefix=<path>::
> Prepend the provided path to the output of each file
> ...
> static int show_eol;
> -static const char *output_path_prefix;
> +static const char *submodule_prefix;
> static int recurse_submodules;
> ...
> static struct strbuf full_name = STRBUF_INIT;
> - if (output_path_prefix && *output_path_prefix) {
> + if (submodule_prefix && *submodule_prefix) {
> strbuf_reset(&full_name);
> - strbuf_addstr(&full_name, output_path_prefix);
> + strbuf_addstr(&full_name, submodule_prefix);
> strbuf_addstr(&full_name, name);
As the previous one that used a wrong (sorry) argument is not even
in 'next' yet, let's pretend that it never happened. It is OK to
still keep it and this patch as two separate steps, i.e. a topic
with two patches in it.
> + /* Add pathspec args */
> + argv_array_push(&cp.args, "--");
> + for (i = 0; i < pathspec.nr; ++i)
> + argv_array_push(&cp.args, pathspec.items[i].original);
OK, so as discussed previously with Heiko and Stefan, the idea is to
- pass the original pathspec as-is,
- when --submodule-prefix is given, a path discovered in a
submodule repository is first prefixed with that string before
getting checked to see if it matches the original pathspec.
And this loop is about relaying the original pathspec.
> @@ -192,57 +210,63 @@ static void show_gitlink(const struct cache_entry *ce)
>
> static void show_ce_entry(const char *tag, const struct cache_entry *ce)
> {
> + struct strbuf name = STRBUF_INIT;
> int len = max_prefix_len;
> + if (submodule_prefix)
> + strbuf_addstr(&name, submodule_prefix);
> + strbuf_addstr(&name, ce->name);
>
> if (len >= ce_namelen(ce))
> - die("git ls-files: internal error - cache entry not superset of prefix");
> + die("git ls-files: internal error - cache entry not "
> + "superset of prefix");
This is not such a great thing to do. Upon a bug report, we can no
longer do
git grep 'cache entry not superset'
to see where the error message is coming from.
> - if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
> - len, ps_matched,
> - S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
> - return;
> - if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
> + if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
> + submodule_path_match(&pathspec, name.buf, ps_matched)) {
> show_gitlink(ce);
> - return;
> - }
> + } else if (match_pathspec(&pathspec, name.buf, name.len,
> + len, ps_matched,
> + S_ISDIR(ce->ce_mode) ||
> + S_ISGITLINK(ce->ce_mode))) {
> + if (tag && *tag && show_valid_bit &&
> + ...
Argh. If we had a preparatory clean-up step, would it have helped
to avoid this big re-indentation that makes the patch harder to read
than necessary, I wonder?
Another way would have been to "goto" from the end of this block
> + if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
> + submodule_path_match(&pathspec, name.buf, ps_matched)) {
where we used to "return" out to the central clean-up location, i.e.
here.
> + strbuf_release(&name);
> }
> parse_pathspec(&pathspec, 0,
> PATHSPEC_PREFER_CWD |
> PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
> prefix, argv);
>
> - /* Find common prefix for all pathspec's */
> - max_prefix = common_prefix(&pathspec);
> + /*
> + * Find common prefix for all pathspec's
> + * This is used as a performance optimization which violates correctness
> + * in the recurse_submodules mode
> + */
The two new lines phrase it overly negatively and also misleading.
I thought you were saying "We do this as optimization anyway; damn
the correctness in the submodule case!" in my first reading before
reading the statements the comment talks about. "This optimization
unfortunately cannot be done when recursing into submodules" would
have been better.
> + if (recurse_submodules)
> + max_prefix = NULL;
> + else
> + max_prefix = common_prefix(&pathspec);
> max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
> diff --git a/dir.c b/dir.c
> index 0ea235f..630dc7a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -63,6 +63,30 @@ int fspathncmp(const char *a, const char *b, size_t count)
> return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
> }
>
> +static int prefix_fnmatch(const struct pathspec_item *item,
> + const char *pattern, const char *string,
> + int prefix)
> +{
Is this meant to be free of false positives, free of false
negatives, or exact? I think you use it to decide, without knowing
what kind of paths the submodule contains, if it is worth descending
into it, so as long as you definitively say "The pathspec can never
match anything in the submodule" with WM_NOMATCH, it is OK if you
returned WM_MATCH when it actually couldn't match anything. I.e. it
is OK to give false positive but it is a bug to give false negative.
The answer to the above question should be a good explanation to
prepend as /* comment */ before the function.
> + if (prefix > 0) {
> + if (ps_strncmp(item, pattern, string, prefix))
> + return WM_NOMATCH;
This says: when we have a set prefix that must literally match, and
that part does not match what we have, it cannot possibly match.
Is that correct? What do we have in "name" and "item" at this
point? We disable the common-prefix optimization, so we do not have
to worry about a pathspec with two elements "sub/dir1/*" and "sub/dir2/*"
giving you "sub/dir" as the common prefix, when you are wondering if
it is worth descending into "sub/" without knowing what it contains.
Is that what guarantees why this part is correct?
> + pattern += prefix;
> + string += prefix;
> + }
> +
> + if (item->flags & PATHSPEC_ONESTAR) {
> + return WM_MATCH;
We have a pathspec that has a segment without wildcard letters,
followed by a '*', and there is no wildcard letters after that
asterisk. We punt and assume it might match, which is OK for the
purpose of not giving a false negative.
> + } else if (item->magic & PATHSPEC_GLOB) {
> + return wildmatch(pattern, string,
> + WM_PATHNAME |
> + (item->magic & PATHSPEC_ICASE ?
> + WM_CASEFOLD : 0),
> + NULL);
What does this say? If we are using the :(glob) semantics, which is
the default, we'll ask wildmatch() to see the remainder of the
pattern (after stripping the fixed prefix part if necessary) matches
the string (which also may have lost the prefix that we already know
matches).
Is that correct? I think it depends on what "string" is being fed,
but I am assuing that you are working in the top-level project here
to decide if it is worth descending into a submodule. If the item
is sub/dir?/*.c and we are considering "sub/" submodule, wildmatch
would not say "It could match" if "string" is "sub/". Perhaps I am
reading the patch incorrectly. Let me read on to see what the caller
does later.
^ permalink raw reply
* [PATCH] ls-files: add pathspec matching for submodules
From: Brandon Williams @ 2016-09-17 0:59 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <CAKoko1pewoxD4=_9M45pchdDg03K8fc73raJOsf4A+=KKw_EMw@mail.gmail.com>
Pathspecs can be a bit tricky when trying to apply them to submodules.
This change permits the pathspec logic to perform a prefix match against
submodules since a pathspec could refer to a file inside of a submodule.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/git-ls-files.txt | 4 +-
builtin/ls-files.c | 143 +++++++++++++++++++--------------
dir.c | 62 +++++++++++++-
dir.h | 4 +
t/t3007-ls-files-recurse-submodules.sh | 66 +++++++++++++--
5 files changed, 208 insertions(+), 71 deletions(-)
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index a623ebf..09e4449 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -19,7 +19,7 @@ SYNOPSIS
[--exclude-standard]
[--error-unmatch] [--with-tree=<tree-ish>]
[--full-name] [--recurse-submodules]
- [--output-path-prefix=<path>]
+ [--submodule-prefix=<path>]
[--abbrev] [--] [<file>...]
DESCRIPTION
@@ -143,7 +143,7 @@ a space) at the start of each line:
Recursively calls ls-files on each submodule in the repository.
Currently there is only support for the --cached mode.
---output-path-prefix=<path>::
+--submodule-prefix=<path>::
Prepend the provided path to the output of each file
--abbrev[=<n>]::
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 687e475..dc1e076 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -29,7 +29,7 @@ static int show_valid_bit;
static int line_terminator = '\n';
static int debug_mode;
static int show_eol;
-static const char *output_path_prefix;
+static const char *submodule_prefix;
static int recurse_submodules;
static const char *prefix;
@@ -78,9 +78,9 @@ static void write_name(const char *name)
* churn.
*/
static struct strbuf full_name = STRBUF_INIT;
- if (output_path_prefix && *output_path_prefix) {
+ if (submodule_prefix && *submodule_prefix) {
strbuf_reset(&full_name);
- strbuf_addstr(&full_name, output_path_prefix);
+ strbuf_addstr(&full_name, submodule_prefix);
strbuf_addstr(&full_name, name);
name = full_name.buf;
}
@@ -177,12 +177,30 @@ static void show_gitlink(const struct cache_entry *ce)
{
struct child_process cp = CHILD_PROCESS_INIT;
int status;
+ int i;
argv_array_push(&cp.args, "ls-files");
argv_array_push(&cp.args, "--recurse-submodules");
- argv_array_pushf(&cp.args, "--output-path-prefix=%s%s/",
- output_path_prefix ? output_path_prefix : "",
+ argv_array_pushf(&cp.args, "--submodule-prefix=%s%s/",
+ submodule_prefix ? submodule_prefix : "",
ce->name);
+ /* add options */
+ if (show_eol)
+ argv_array_push(&cp.args, "--eol");
+ if (show_valid_bit)
+ argv_array_push(&cp.args, "-v");
+ if (show_stage)
+ argv_array_push(&cp.args, "--stage");
+ if (show_cached)
+ argv_array_push(&cp.args, "--cached");
+ if (debug_mode)
+ argv_array_push(&cp.args, "--debug");
+
+ /* Add pathspec args */
+ argv_array_push(&cp.args, "--");
+ for (i = 0; i < pathspec.nr; ++i)
+ argv_array_push(&cp.args, pathspec.items[i].original);
+
cp.git_cmd = 1;
cp.dir = ce->name;
status = run_command(&cp);
@@ -192,57 +210,63 @@ static void show_gitlink(const struct cache_entry *ce)
static void show_ce_entry(const char *tag, const struct cache_entry *ce)
{
+ struct strbuf name = STRBUF_INIT;
int len = max_prefix_len;
+ if (submodule_prefix)
+ strbuf_addstr(&name, submodule_prefix);
+ strbuf_addstr(&name, ce->name);
if (len >= ce_namelen(ce))
- die("git ls-files: internal error - cache entry not superset of prefix");
+ die("git ls-files: internal error - cache entry not "
+ "superset of prefix");
- if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
- len, ps_matched,
- S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
- return;
- if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+ if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+ submodule_path_match(&pathspec, name.buf, ps_matched)) {
show_gitlink(ce);
- return;
- }
+ } else if (match_pathspec(&pathspec, name.buf, name.len,
+ len, ps_matched,
+ S_ISDIR(ce->ce_mode) ||
+ S_ISGITLINK(ce->ce_mode))) {
+ if (tag && *tag && show_valid_bit &&
+ (ce->ce_flags & CE_VALID)) {
+ static char alttag[4];
+ memcpy(alttag, tag, 3);
+ if (isalpha(tag[0]))
+ alttag[0] = tolower(tag[0]);
+ else if (tag[0] == '?')
+ alttag[0] = '!';
+ else {
+ alttag[0] = 'v';
+ alttag[1] = tag[0];
+ alttag[2] = ' ';
+ alttag[3] = 0;
+ }
+ tag = alttag;
+ }
- if (tag && *tag && show_valid_bit &&
- (ce->ce_flags & CE_VALID)) {
- static char alttag[4];
- memcpy(alttag, tag, 3);
- if (isalpha(tag[0]))
- alttag[0] = tolower(tag[0]);
- else if (tag[0] == '?')
- alttag[0] = '!';
- else {
- alttag[0] = 'v';
- alttag[1] = tag[0];
- alttag[2] = ' ';
- alttag[3] = 0;
+ if (!show_stage) {
+ fputs(tag, stdout);
+ } else {
+ printf("%s%06o %s %d\t",
+ tag,
+ ce->ce_mode,
+ find_unique_abbrev(ce->sha1,abbrev),
+ ce_stage(ce));
+ }
+ write_eolinfo(ce, ce->name);
+ write_name(ce->name);
+ if (debug_mode) {
+ const struct stat_data *sd = &ce->ce_stat_data;
+
+ printf(" ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
+ printf(" mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
+ printf(" dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
+ printf(" uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
+ printf(" size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
}
- tag = alttag;
}
- if (!show_stage) {
- fputs(tag, stdout);
- } else {
- printf("%s%06o %s %d\t",
- tag,
- ce->ce_mode,
- find_unique_abbrev(ce->sha1,abbrev),
- ce_stage(ce));
- }
- write_eolinfo(ce, ce->name);
- write_name(ce->name);
- if (debug_mode) {
- const struct stat_data *sd = &ce->ce_stat_data;
-
- printf(" ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
- printf(" mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
- printf(" dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
- printf(" uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
- printf(" size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
- }
+ strbuf_release(&name);
}
static void show_ru_info(void)
@@ -510,7 +534,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
N_("make the output relative to the project top directory"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
- OPT_STRING(0, "output-path-prefix", &output_path_prefix,
+ OPT_STRING(0, "submodule-prefix", &submodule_prefix,
N_("path"), N_("prepend <path> to each file")),
OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
N_("recurse through submodules")),
@@ -566,27 +590,28 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
setup_work_tree();
if (recurse_submodules &&
- (show_stage || show_deleted || show_others || show_unmerged ||
- show_killed || show_modified || show_resolve_undo ||
- show_valid_bit || show_tag || show_eol))
- die("ls-files --recurse-submodules can only be used in "
- "--cached mode");
+ (show_deleted || show_others || show_unmerged ||
+ show_killed || show_modified || show_resolve_undo))
+ die("ls-files --recurse-submodules unsupported mode");
if (recurse_submodules && error_unmatch)
die("ls-files --recurse-submodules does not support "
"--error-unmatch");
- if (recurse_submodules && argc)
- die("ls-files --recurse-submodules does not support path "
- "arguments");
-
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
prefix, argv);
- /* Find common prefix for all pathspec's */
- max_prefix = common_prefix(&pathspec);
+ /*
+ * Find common prefix for all pathspec's
+ * This is used as a performance optimization which violates correctness
+ * in the recurse_submodules mode
+ */
+ if (recurse_submodules)
+ max_prefix = NULL;
+ else
+ max_prefix = common_prefix(&pathspec);
max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
/* Treat unmatching pathspec elements as errors */
diff --git a/dir.c b/dir.c
index 0ea235f..630dc7a 100644
--- a/dir.c
+++ b/dir.c
@@ -63,6 +63,30 @@ int fspathncmp(const char *a, const char *b, size_t count)
return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
}
+static int prefix_fnmatch(const struct pathspec_item *item,
+ const char *pattern, const char *string,
+ int prefix)
+{
+ if (prefix > 0) {
+ if (ps_strncmp(item, pattern, string, prefix))
+ return WM_NOMATCH;
+ pattern += prefix;
+ string += prefix;
+ }
+
+ if (item->flags & PATHSPEC_ONESTAR) {
+ return WM_MATCH;
+ } else if (item->magic & PATHSPEC_GLOB) {
+ return wildmatch(pattern, string,
+ WM_PATHNAME |
+ (item->magic & PATHSPEC_ICASE ?
+ WM_CASEFOLD : 0),
+ NULL);
+ }
+
+ return WM_NOMATCH;
+}
+
int git_fnmatch(const struct pathspec_item *item,
const char *pattern, const char *string,
int prefix)
@@ -207,8 +231,9 @@ int within_depth(const char *name, int namelen,
return 1;
}
-#define DO_MATCH_EXCLUDE 1
-#define DO_MATCH_DIRECTORY 2
+#define DO_MATCH_EXCLUDE (1<<0)
+#define DO_MATCH_DIRECTORY (1<<1)
+#define DO_MATCH_SUBMODULE (1<<2)
/*
* Does 'match' match the given name?
@@ -283,6 +308,24 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
item->nowildcard_len - prefix))
return MATCHED_FNMATCH;
+ /* Perform checks to see if "name" is a super set of the pathspec */
+ if (flags & DO_MATCH_SUBMODULE) {
+ int matched = 0;
+
+ /* Check if the name is a literal prefix of the pathspec */
+ if ((item->match[namelen] == '/') &&
+ !ps_strncmp(item, match, name, namelen)) {
+ matched = MATCHED_RECURSIVELY;
+ /* Check if the name wildmatches to the pathspec */
+ } else if (item->nowildcard_len < item->len &&
+ !prefix_fnmatch(item, match, name,
+ item->nowildcard_len - prefix)) {
+ matched = MATCHED_FNMATCH;
+ }
+
+ return matched;
+ }
+
return 0;
}
@@ -386,6 +429,21 @@ int match_pathspec(const struct pathspec *ps,
return negative ? 0 : positive;
}
+/**
+ * Check if a submodule is a superset of the pathspec
+ */
+int submodule_path_match(const struct pathspec *ps,
+ const char *submodule_name,
+ char *seen)
+{
+ int matched = do_match_pathspec(ps, submodule_name,
+ strlen(submodule_name),
+ 0, seen,
+ DO_MATCH_DIRECTORY |
+ DO_MATCH_SUBMODULE);
+ return matched;
+}
+
int report_path_error(const char *ps_matched,
const struct pathspec *pathspec,
const char *prefix)
diff --git a/dir.h b/dir.h
index da1a858..97c83bb 100644
--- a/dir.h
+++ b/dir.h
@@ -304,6 +304,10 @@ extern int git_fnmatch(const struct pathspec_item *item,
const char *pattern, const char *string,
int prefix);
+extern int submodule_path_match(const struct pathspec *ps,
+ const char *submodule_name,
+ char *seen);
+
static inline int ce_path_match(const struct cache_entry *ce,
const struct pathspec *pathspec,
char *seen)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index caf3815..977f85c 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -69,9 +69,63 @@ test_expect_success 'ls-files recurses more than 1 level' '
test_cmp expect actual
'
-test_expect_success '--recurse-submodules does not support using path arguments' '
- test_must_fail git ls-files --recurse-submodules b 2>actual &&
- test_i18ngrep "does not support path arguments" actual
+test_expect_success '--recurse-submodules and pathspecs setup' '
+ echo e >submodule/subsub/e.txt &&
+ git -C submodule/subsub add e.txt &&
+ git -C submodule/subsub commit -m "adding e.txt" &&
+ echo f >submodule/f.TXT &&
+ echo g >submodule/g.txt &&
+ git -C submodule add f.TXT g.txt &&
+ git -C submodule commit -m "add f and g" &&
+ echo h >h.txt &&
+ git add h.txt &&
+ git commit -m "add h" &&
+
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ h.txt
+ submodule/.gitmodules
+ submodule/c
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/d
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules "*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/g.txt
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules "*.txt" >actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules ":(icase)*.txt" >actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/f.TXT
+ submodule/g.txt
+ EOF
+
+ git ls-files --recurse-submodules ":(icase)*.txt" ":(exclude)submodule/subsub/*" >actual &&
+ test_cmp expect actual
'
test_expect_success '--recurse-submodules does not support --error-unmatch' '
@@ -82,18 +136,14 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '
test_incompatible_with_recurse_submodules () {
test_expect_success "--recurse-submodules and $1 are incompatible" "
test_must_fail git ls-files --recurse-submodules $1 2>actual &&
- test_i18ngrep 'can only be used in --cached mode' actual
+ test_i18ngrep 'unsupported mode' actual
"
}
-test_incompatible_with_recurse_submodules -v
-test_incompatible_with_recurse_submodules -t
test_incompatible_with_recurse_submodules --deleted
test_incompatible_with_recurse_submodules --modified
test_incompatible_with_recurse_submodules --others
-test_incompatible_with_recurse_submodules --stage
test_incompatible_with_recurse_submodules --killed
test_incompatible_with_recurse_submodules --unmerged
-test_incompatible_with_recurse_submodules --eol
test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH] format-patch: Add --rfc for the common case of [RFC PATCH]
From: Jeff King @ 2016-09-17 0:49 UTC (permalink / raw)
To: Josh Triplett; +Cc: git, Andrew Donnellan
In-Reply-To: <28c5d2c59851279858df22e844c6ff7c09f33199.1474046573.git-series.josh@joshtriplett.org>
On Fri, Sep 16, 2016 at 10:27:45AM -0700, Josh Triplett wrote:
> By far, the most common subject-prefix I've seen other than "PATCH" is
> "RFC PATCH" (or occasionally "PATCH RFC"). Seems worth optimizing for
> the common case, to avoid having to spell it out the long way as
> --subject-prefix='RFC PATCH'.
"RFC" is the most common one for me, too. And if it ends here, I'm OK
with it. But I'm a little worried with ending up with a proliferation of
options.
If we had a short-option for --subject-prefix, then:
-P RFC
is not so bad compared to "--rfc". But if you want to spell it as "RFC
PATCH" that's getting a bit longer. We could have a short option for
"tag this in the subject prefix _in addition_ to writing PATCH". And
then you could do:
-T RFC
I dunno. One other thing to consider is that format-patch takes
arbitrary diff options, so we'd want to avoid stomping on them with any
short options (which is why I used "-T" instead of "-t", though I find
it unlikely that many people use the latter with format-patch). That's a
point in favor of --rfc, I think.
> builtin/log.c | 10 ++++++++++
> t/t4014-format-patch.sh | 9 +++++++++
> 2 files changed, 19 insertions(+), 0 deletions(-)
Documentation?
> +static int rfc_callback(const struct option *opt, const char *arg, int unset)
> +{
> + subject_prefix = 1;
> + ((struct rev_info *)opt->value)->subject_prefix = xstrdup("RFC PATCH");
> + return 0;
> +}
I was going to complain that you don't free() the previous value, but
actually the other callers do not xstrdup() in the first place (and we
do not need to do so here, either, as it's a string literal). We
actually _do_ allocate a new copy when reading the value from config,
but it's probably not a big deal in practice to leak that.
I also wonder if you could implement this as just:
return subject_prefix_callback(opt, "RFC PATCH", unset);
And then if you write the documentation as:
--rfc::
Behave as if --subject-prefix="RFC PATCH" was specified.
then it will be trivially correct. :)
> +cat >expect <<'EOF'
> +Subject: [RFC PATCH 1/1] header with . in it
> +EOF
> +test_expect_success '--rfc' '
> + git format-patch -n -1 --stdout --rfc >patch &&
> + grep ^Subject: patch >actual &&
> + test_cmp expect actual
> +'
Our usual style these days is to set up expectations inside the test
blocks (and use "<<-" to get nice indentation; we also typically use
"\EOF" but that's purely style).
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 2/3] mailinfo: correct malformed test example
From: Jonathan Tan @ 2016-09-17 0:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff
In-Reply-To: <xmqqbmznihe0.fsf@gitster.mtv.corp.google.com>
On 09/16/2016 03:55 PM, Junio C Hamano wrote:
> Hmph, these:
>
> t/t5100/info0008--no-inbody-headers | 5 +++++
> t/t5100/msg0008--no-inbody-headers | 6 ++++++
> t/t5100/msg0015--no-inbody-headers | 1 +
>
> have --no-inbody-headers in their names; wouldn't that an indication
> that they are expected output when mailinfo is run while in-body
> header feature disabled?
Yes, that's correct (they are the test data for when the in-body header
feature is disabled).
> I would have expected that it would make more sense to make no
> change to sample.mbox and have updated expectation to outputs in the
> case where in-body header feature is enabled.
The sample.mbox file contains the following:
From nobody Mon Sep 17 00:00:00 2001
From: A U Thor <a.u.thor@example.com>
Subject: check bogus body header (from)
Date: Fri, 9 Jun 2006 00:44:16 -0700
From: bogosity
- a list
- of stuff
Unchanged, the subsequent patch would break this test because it would
interpret that as a multi-line "From" in-body header when in-body
headers are *not* disabled.
Besides changing sample.mbox, the other way to make sure that this test
passes is to suppress the test when in-body headers are *not* disabled,
but looking at t5100* (directory and script), it seemed more
straightforward to modify sample.mbox.
The patch I sent added a blank line after "From: bogosity", but removing
the spaces before "- a list" and "- of stuff" would work too.
> To make sure this new feature will not break in the future, we would
> want a brand new message with a folded in-body header added to the
> sample.mbox, and see how it is parsed by mailinfo with in-body
> header feature enabled (and disabled).
OK, I'll add this test. (The subsequent patch already has the brand new
message, but not the test where in-body headers are disabled.)
^ permalink raw reply
* Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations
From: Jonathan Tan @ 2016-09-17 0:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff
In-Reply-To: <xmqq7fabigzf.fsf@gitster.mtv.corp.google.com>
On 09/16/2016 04:04 PM, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>> I'm concerned about what happens if check_header fails - we would then
>> have some lines which need to be treated as log messages. (At least,
>> they are currently treated that way.)
>
> I actually think we should refactor check_header() further so that
> in-body header processing does not even see things that shouldn't be
> changed. The current check_header() should be used only for real
> mail headers, and then a reduced version of check_header() that is
> called for in-body will _ONLY_ handle the header lines that are
> handled by the first "search for the interesting parts" loop.
>
> And of course we would update your "does it look like rfc2822?" to
> match what are handled by the "interesting parts" loop. That I
> think would match the current behaviour much better, I would think.
There would be a bit of code duplication in that this "does it look like
rfc2822" function would also need to account for duplicate headers (e.g.
2 "Subject:" lines in the in-body headers) because check_header would
reject the 2nd one, but that is minor. (Alternatively, we could just
allow duplicate headers in the in-body headers.)
> The ">From " and "[PATCH]" cases in check_header() should not even
> be there. We should handle them inside handle_commit_msg(), as
> these two cases should never appear in the real header part of a
> message.
> And if we clean it up like that, I do not think we would ever need
> to worry about "ah, it looked like a header but it is not after
> all". And not having to worry about it is a good thing and should
> be one of the primary goals in this conversion, I whould think.
Yes, this makes sense. I'll go ahead and make a patch set implementing
this (unless anyone has any objections).
^ permalink raw reply
* Re: [PATCH 11/11] Resumable clone: implement primer logic in git-clone
From: Junio C Hamano @ 2016-09-16 23:32 UTC (permalink / raw)
To: Kevin Wern; +Cc: git
In-Reply-To: <1473984742-12516-12-git-send-email-kevin.m.wern@gmail.com>
Kevin Wern <kevin.m.wern@gmail.com> writes:
> Use transport_download_primer and transport_prime_clone in git clone.
> This only supports a fully connected packfile.
>
> transport_prime_clone and transport_download_primer are executed
> completely independent of transport_(get|fetch)_remote_refs, et al.
> transport_download_primer is executed based on the existence of an
> alt_resource. The idea is that the "prime clone" execution should be
> able to attempt retrieving an alternate resource without dying, as
> opposed to depending on the result of upload pack's "capabilities" to
> indicate whether or not the client can attempt it.
>
> If a resumable resource is available, execute a codepath with the
> following modular components:
> - downloading resource to a specific directory
> - using the resource (for pack, indexing and generating the bundle
> file)
> - cleaning up the resource (if the download or use fails)
> - cleaning up the resource (if the download or use succeeds)
>
> If resume is interrupted on the client side, the alternate resource
> info is written to the RESUMABLE file in the git directory.
>
> On resume, the required info is extracted by parsing the created
> config file, and that info is used to determine the work and git
> directories. If these cannot be determined, the program exits.
> The writing of the refspec and determination of the initial git
> directories are skipped, along with transport_prime_clone.
>
> The main purpose of this series of patches is to flesh out a codepath
> for automatic resuming, manual resuming, and leaving a resumable
> directory on exit--the logic for when to do these still needs more
> work.
>
> Signed-off-by: Kevin Wern <kevin.m.wern@gmail.com>
> ---
> Documentation/git-clone.txt | 16 ++
> builtin/clone.c | 590 +++++++++++++++++++++++++++++++++++++-------
> t/t9904-git-prime-clone.sh | 181 ++++++++++++++
> 3 files changed, 698 insertions(+), 89 deletions(-)
> create mode 100755 t/t9904-git-prime-clone.sh
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index b7c467a..5934bb6 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -16,6 +16,7 @@ SYNOPSIS
> [--depth <depth>] [--[no-]single-branch]
> [--recursive | --recurse-submodules] [--] <repository>
> [<directory>]
> +'git clone --resume <resumable_dir>'
>
> DESCRIPTION
> -----------
> @@ -172,6 +173,12 @@ objects from the source repository into a pack in the cloned repository.
> via ssh, this specifies a non-default path for the command
> run on the other end.
>
> +--prime-clone <prime-clone>::
> +-p <prime-clone>::
Not many other options have single letter shorthand. Is it expected
that it is worth to let this option squat on a short-and-sweet "-p",
perhaps because it is so frequently used?
> +--resume::
> + Resume a partially cloned repo in a "resumable" state. This
> + can only be specified with a single local directory (<resumable
> + dir>). This is incompatible with all other options.
> +
> +<resumable_dir>::
> + The directory of the partial clone. This could be either the
> + work directory or the git directory.
I think these should be described this way:
--resume <resumable_dir>::
description if what resume option does and how resumable_dir
is used by the option.
in a single bullet point.
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 9ac6c01..d9a13dc 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -8,7 +8,9 @@
> * Clone a repository into a different directory that does not yet exist.
> */
>
> +#include "cache.h"
> #include "builtin.h"
I do not think you need to include cache.h if you are including
builtin.h; Documentation/CodingGuidelines says:
- The first #include in C files, except in platform specific compat/
implementations, must be either "git-compat-util.h", "cache.h" or
"builtin.h". You do not have to include more than one of these.
> @@ -40,17 +42,20 @@ static const char * const builtin_clone_usage[] = {
>
> static int option_no_checkout, option_bare, option_mirror, option_single_branch = -1;
> static int option_local = -1, option_no_hardlinks, option_shared, option_recursive;
> +static int option_resume;
> static char *option_template, *option_depth;
> -static char *option_origin = NULL;
> +static const char *option_origin = NULL;
Is this change related to anything you are doing here?
If you are fixing things while at it, please don't ;-) If you really
want to, please also remove " = NULL", from this line and also from
the next line. Also do not add " = NULL" at the end of alt_res.
> static char *option_branch = NULL;
> ...
> +static const struct alt_resource *alt_res = NULL;
> +static char *get_filename(const char *dir)
> +{
> + char *dir_copy = xstrdup(dir);
> + strip_trailing_slashes(dir_copy);
> + char *filename, *final = NULL;
> +
> + filename = find_last_dir_sep(dir);
> +
> + if (filename && *(++filename))
> + final = xstrdup(filename);
> +
> + free(dir_copy);
> + return final;
> +}
Hmph, don't we have our own basename(3) lookalike that knows about
dir-sep already?
> @@ -451,6 +475,7 @@ static const char *junk_work_tree;
> static const char *junk_git_dir;
> static enum {
> JUNK_LEAVE_NONE,
> + JUNK_LEAVE_RESUMABLE,
> JUNK_LEAVE_REPO,
> JUNK_LEAVE_ALL
> } junk_mode = JUNK_LEAVE_NONE;
> @@ -460,6 +485,29 @@ N_("Clone succeeded, but checkout failed.\n"
> "You can inspect what was checked out with 'git status'\n"
> "and retry the checkout with 'git checkout -f HEAD'\n");
>
> +static const char junk_leave_resumable_msg[] =
> +N_("Clone interrupted while copying resumable resource.\n"
> + "Try using 'git clone --resume <new_directory>',\n"
> + "where <new_directory> is either the new working \n"
> + "directory or git directory.\n\n"
> + "If this does not succeed, it could be because the\n"
> + "resource has been moved, corrupted, or changed.\n"
> + "If this is the case, you should remove <new_directory>\n"
> + "and run the original command.\n");
> +
> +static void write_resumable_resource()
> +{
> + const char *filename = git_path_resumable();
> + struct strbuf content = STRBUF_INIT;
> + strbuf_addf(&content, "%s\n%s\n", alt_res->url, alt_res->filetype);
> + int fd = open(filename, O_WRONLY | O_CREAT, 0666);
> + if (fd < 0)
> + die_errno(_("Could not open '%s' for writing"), filename);
> + if (write_in_full(fd, content.buf, content.len) != content.len)
> + die_errno(_("Could not write to '%s'"), filename);
> + close(fd);
> +}
>
> static void remove_junk(void)
> {
> struct strbuf sb = STRBUF_INIT;
> @@ -467,7 +515,11 @@ static void remove_junk(void)
> switch (junk_mode) {
> case JUNK_LEAVE_REPO:
> warning("%s", _(junk_leave_repo_msg));
> - /* fall-through */
> + return;
> + case JUNK_LEAVE_RESUMABLE:
> + write_resumable_resource();
> + warning("%s", _(junk_leave_resumable_msg));
> + return;
Nice.
> @@ -562,7 +614,7 @@ static void write_remote_refs(const struct ref *local_refs)
> die("%s", err.buf);
>
> for (r = local_refs; r; r = r->next) {
> - if (!r->peer_ref)
> + if (!r->peer_ref || ref_exists(r->peer_ref->name))
> continue;
> if (ref_transaction_create(t, r->peer_ref->name, r->old_oid.hash,
> 0, NULL, &err))
What is this change about?
> @@ -820,11 +872,296 @@ static void dissociate_from_references(void)
> free(alternates);
> }
>
> +static int do_index_pack(const char *in_pack_file, const char *out_idx_file)
> +{
> + const char *argv[] = { "index-pack", "--clone-bundle", "-v",
> + "--check-self-contained-and-connected", "-o",
> + out_idx_file, in_pack_file, NULL };
> + return run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDOUT);
> +}
That looks vaguely familiar ;-)
> +static const char *setup_and_index_pack(const char *filename)
> +{
> + const char *primer_idx_path = NULL, *primer_bndl_path = NULL;
> + primer_idx_path = replace_extension(filename, ".pack", ".idx");
> + primer_bndl_path = replace_extension(filename, ".pack", ".bndl");
> +
> + if (!(primer_idx_path && primer_bndl_path)) {
> + warning("invalid pack filename '%s', falling back to full "
> + "clone", filename);
> + return NULL;
> + }
> +
> + if (!file_exists(primer_bndl_path)) {
> + if (do_index_pack(filename, primer_idx_path)) {
> + warning("could not index primer pack, falling back to "
> + "full clone");
> + return NULL;
> + }
> + }
Can it be another (undetected) failure mode that .bndl somehow
already existed, but not .idx, leaving the resulting object store in
an incosistent state? Can do_index_pack() fail and leave .bndl
behind to get you into such a state?
> +static int write_bundle_refs(const char *bundle_filename)
> +{
> + struct ref_transaction *t;
> + struct bundle_header history_tips;
> + const char *temp_ref_base = "resume";
> + struct strbuf err = STRBUF_INIT;
> + int i;
> +
> + init_bundle_header(&history_tips, bundle_filename);
> + read_bundle_header(&history_tips);
> +
> + t = ref_transaction_begin(&err);
> + for (i = 0; i < history_tips.references.nr; i++) {
> + struct strbuf ref_name = STRBUF_INIT;
> + strbuf_addf(&ref_name, "refs/temp/%s/%s/temp-%s",
> + option_origin, temp_ref_base,
> + sha1_to_hex(history_tips.references.list[i].sha1));
Can we do this without polluting refs/temp/ namespace?
I am imagining that you are first fetching the .pack file from
sideways when primer service is available, running index-pack on it
to produce the bundle, and the step after that is to run "git fetch"
against the original remote to fill the gap between the bit-stale
history you got in the bundle and the reality that has progressed
since the primer pack was made, and you need a way to tell to the
other end that you already have the history leading to these refs
when you run "git fetch". I think a bit better way to do so is to
send these has ".have" while you run the "fetch".
Wouldn't it do if you add the "--advertise-bundle-tips=<bndl>"
option to "git fetch", move the code to read the bundle header to
it, and point the bundle's filename with the option when you spawn
"git fetch"?
^ permalink raw reply
* Re: [PATCH 10/11] run command: add RUN_COMMAND_NO_STDOUT
From: Junio C Hamano @ 2016-09-16 23:07 UTC (permalink / raw)
To: Kevin Wern; +Cc: git
In-Reply-To: <1473984742-12516-11-git-send-email-kevin.m.wern@gmail.com>
Kevin Wern <kevin.m.wern@gmail.com> writes:
> Add option RUN_COMMAND_NO_STDOUT, which sets no_stdout on a child
> process.
>
> This will be used by git clone when calling index-pack on a downloaded
> packfile.
If it is just one caller, would't it make more sense for that caller
set no_stdout explicitly itself?
^ permalink raw reply
* Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations
From: Junio C Hamano @ 2016-09-16 23:04 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, peff
In-Reply-To: <128036e3-47b4-b7ae-034f-8d227a62729f@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> On 09/16/2016 01:59 PM, Junio C Hamano wrote:
>> if (mi->in_line_header->len) {
>> /* we have read the beginning of one in-line header */
>> if (line->len && isspace(*line->buf) &&
>> !(mi->use_scissors && is_scissors_line(line))) {
>
> Minor note: this means that the scissors check appears twice in the
> code, once here and once below (for the non-header case).
Yes. I actually was wondering if it is even more sensible to always
have the scissors check at the very beginning. Even if we saw a
half-written in-body header already in the message, when we see a
scissors line, we clear the slate and restart as if the line after
the scissors is the first line in the body of the message.
>> append to mi->in_line_header strbuf;
>> return 0;
>> }
>> /* otherwise we know mi->in_line_header is now complete */
>> check_header(mi, mi->in_line_header, ...);
>
> (Sorry - should have also noticed this in your original e-mail.)
>
> I'm concerned about what happens if check_header fails - we would then
> have some lines which need to be treated as log messages. (At least,
> they are currently treated that way.)
I actually think we should refactor check_header() further so that
in-body header processing does not even see things that shouldn't be
changed. The current check_header() should be used only for real
mail headers, and then a reduced version of check_header() that is
called for in-body will _ONLY_ handle the header lines that are
handled by the first "search for the interesting parts" loop.
And of course we would update your "does it look like rfc2822?" to
match what are handled by the "interesting parts" loop. That I
think would match the current behaviour much better, I would think.
The ">From " and "[PATCH]" cases in check_header() should not even
be there. We should handle them inside handle_commit_msg(), as
these two cases should never appear in the real header part of a
message.
And if we clean it up like that, I do not think we would ever need
to worry about "ah, it looked like a header but it is not after
all". And not having to worry about it is a good thing and should
be one of the primary goals in this conversion, I whould think.
Thanks.
^ permalink raw reply
* Re: [PATCH 03/10] diff.c: drop tautologous condition in emit_line_0
From: Stefan Beller @ 2016-09-16 23:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <xmqq1t0oy8t8.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 12, 2016 at 4:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <stefanbeller@gmail.com> writes:
>
>> diff --git a/diff.c b/diff.c
>> index 156c2aa..9d2e704 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -460,8 +460,7 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
>>
>> if (len == 0) {
>> has_trailing_newline = (first == '\n');
>> - has_trailing_carriage_return = (!has_trailing_newline &&
>> - (first == '\r'));
>> + has_trailing_carriage_return = (first == '\r');
>> nofirst = has_trailing_newline || has_trailing_carriage_return;
>> } else {
>> has_trailing_newline = (len > 0 && line[len-1] == '\n');
>
> Interesting.
>
> This may be a mis-conversion at 250f7993 ("diff.c: split emit_line()
> from the first char and the rest of the line", 2009-09-14), I
> suspect. The original took line[] with length and peeked for '\n',
> and when it saw one, it decremented length before checking
> line[len-1] for '\r'.
>
> But of course if there is only one byte on the line (i.e. len == 0
> after first is stripped off), it cannot be both '\n' or '\r' at the
> same time.
>
> Thanks for spotting.
Oh, right, it used to be possible to remove \r\n completely and that information
was then kept as has_trailing_newline = has_trailing_carriage_return = 1;
and the resulting line is kept completely without ending line.
After some thought I don't think I can use this mis-conversion
to trigger a bug though, because the len=0 can only ever happen
if first is '\n' alone essentially.
Another thing I noticed when playing around with diffs:
$ printf "\r\n" >crlf
$ git commit crlf -m "add file crlf, empty line"
$ printf "non zero length\r\n" >crlf
$ diff --git a/crlf b/crlf
$ index d3f5a12..ece7140 100644
--- a/crlf
+++ b/crlf
@@ -1 +1 @@
-
+non zero length^M
$ # The - line is missing a ^M ?
^ permalink raw reply
* Re: [RFC/PATCH 2/3] mailinfo: correct malformed test example
From: Junio C Hamano @ 2016-09-16 22:55 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, peff
In-Reply-To: <2bfc2fc7-f16b-6d51-7353-54d38353464a@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> On 09/16/2016 12:19 PM, Junio C Hamano wrote:
>> Jonathan Tan <jonathantanmy@google.com> writes:
>>
>>> An existing sample message (0015) in the tests for mailinfo contains an
>>> indented line immediately after an in-body header (without any
>>> intervening blank line).
>>
>> This comes from d25e5159 ("git am/mailinfo: Don't look at in-body
>> headers when rebasing", 2009-11-20), where we want to make sure that
>> a "From: bogosity" that isn't meant to be an in-body header is not
>> identified as such, even when it is immediately followed by a
>> non-blank line. "From: bogosity" is for msg0015 but the same
>> applies to the header-looking block for msg0008.
>>
>> Adding a blank line there will defeat the whole point of the test,
>> which is to make sure we don't do anything funky when --no-inbody-headers
>> is asked for, no?
>
> Before I revise the patch set...I think that the point of 0015 would
> be handled by 0008 (after this patch is applied), but if you prefer
> that 0015 retain its purpose, I can unindent the bullet list in 0015
> instead of adding the extra line (and then dropping all 0008
> changes). Would that be better? (0015 needs to be changed somehow,
> because its indented line would be interpreted as a continuation line
> after RFC/PATCH 3/3 is applied.)
Hmph, these:
t/t5100/info0008--no-inbody-headers | 5 +++++
t/t5100/msg0008--no-inbody-headers | 6 ++++++
t/t5100/msg0015--no-inbody-headers | 1 +
have --no-inbody-headers in their names; wouldn't that an indication
that they are expected output when mailinfo is run while in-body
header feature disabled?
I would have expected that it would make more sense to make no
change to sample.mbox and have updated expectation to outputs in the
case where in-body header feature is enabled.
To make sure this new feature will not break in the future, we would
want a brand new message with a folded in-body header added to the
sample.mbox, and see how it is parsed by mailinfo with in-body
header feature enabled (and disabled).
^ permalink raw reply
* Re: [PATCH 07/11] Resumable clone: add resumable download to http/curl
From: Junio C Hamano @ 2016-09-16 22:45 UTC (permalink / raw)
To: Kevin Wern; +Cc: git
In-Reply-To: <1473984742-12516-8-git-send-email-kevin.m.wern@gmail.com>
Kevin Wern <kevin.m.wern@gmail.com> writes:
> +int http_download_primer(const char *url, const char *out_file)
> +{
> + int ret = 0, try_count = HTTP_TRY_COUNT;
> + struct http_get_options options = {0};
> + options.progress = 1;
> +
> + if (file_exists(out_file)) {
> + fprintf(stderr,
> + "File already downloaded: '%s', skipping...\n",
> + out_file);
> + return ret;
> + }
> +
> + do {
> + if (try_count != HTTP_TRY_COUNT) {
> + fprintf(stderr, "Connection interrupted for some "
> + "reason, retrying (%d attempts left)\n",
> + try_count);
> + struct timeval time = {10, 0}; // 1s
We do not use // comment.
> + select(0, NULL, NULL, NULL, &time);
> + }
> + ret = http_get_file(url, out_file, &options);
I didn't realize that http_get_file() -> http_request() codepath,
when it is the output file, already can do the "ftell and request
the reminder". Very nice.
> @@ -1136,7 +1138,10 @@ static int handle_curl_result(struct slot_results *results)
> curl_easy_strerror(results->curl_result),
> sizeof(curl_errorstr));
> #endif
> - return HTTP_ERROR;
> + if (results->http_code >= 400)
> + return HTTP_ERROR;
> + else
> + return HTTP_ERROR_RESUMABLE;
> }
> }
Hmm, is "anything below 400" a good definition of resumable errors?
^ permalink raw reply
* Re: [RFC/PATCH 2/3] mailinfo: correct malformed test example
From: Jonathan Tan @ 2016-09-16 22:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff
In-Reply-To: <xmqqk2ebk5zh.fsf@gitster.mtv.corp.google.com>
On 09/16/2016 12:19 PM, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> An existing sample message (0015) in the tests for mailinfo contains an
>> indented line immediately after an in-body header (without any
>> intervening blank line).
>
> This comes from d25e5159 ("git am/mailinfo: Don't look at in-body
> headers when rebasing", 2009-11-20), where we want to make sure that
> a "From: bogosity" that isn't meant to be an in-body header is not
> identified as such, even when it is immediately followed by a
> non-blank line. "From: bogosity" is for msg0015 but the same
> applies to the header-looking block for msg0008.
>
> Adding a blank line there will defeat the whole point of the test,
> which is to make sure we don't do anything funky when --no-inbody-headers
> is asked for, no?
Before I revise the patch set...I think that the point of 0015 would be
handled by 0008 (after this patch is applied), but if you prefer that
0015 retain its purpose, I can unindent the bullet list in 0015 instead
of adding the extra line (and then dropping all 0008 changes). Would
that be better? (0015 needs to be changed somehow, because its indented
line would be interpreted as a continuation line after RFC/PATCH 3/3 is
applied.)
^ permalink raw reply
* git-subtree pull issue
From: Alexander Inyukhin @ 2016-09-16 22:39 UTC (permalink / raw)
To: git
When git-subtree is pulling data using the tag reference,
it writes the tag's sha1 into a metadata.
It could be a problem next time, since this commit object
is not a part of main tree, and could be lost.
Steps to reproduce:
# add some stuff to a existing tree
# history will refer to v0.1 tag object instead of v0.1^{commit}
git subtree add --squash -P dir/ repo v0.1
# prune all dangling objects including external v0.1 tag
git gc --aggressive --prune=all
# this will fail
git subtree pull --squash -P dir/ repo v0.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