* Re: [PATCH v3 5/5] stash: teach 'push' (and 'create') to honor pathspec
From: Thomas Gummerer @ 2017-02-11 16:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A . Holm, Jakub Narębski
In-Reply-To: <xmqqmvdz3ied.fsf@gitster.mtv.corp.google.com>
On 02/05, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > @@ -72,6 +72,11 @@ create_stash () {
> > untracked="$1"
> > new_style=t
> > ;;
> > + --)
> > + shift
> > + new_style=t
> > + break
> > + ;;
> > *)
> > if test -n "$new_style"
> > then
> > @@ -99,6 +104,10 @@ create_stash () {
> > if test -z "$new_style"
> > then
> > stash_msg="$*"
> > + while test $# != 0
> > + do
> > + shift
> > + done
> > fi
>
> The intent is correct, but I would probaly empty the "$@" not with
> an iteration of shifts but with a single
>
> set x && shift
>
> ;-)
Thanks, I knew there had to be a better way, but I was unable to find
it. I think I like Andreas suggestion of "shift $#" the best here.
> > @@ -134,7 +143,7 @@ create_stash () {
> > # Untracked files are stored by themselves in a parentless commit, for
> > # ease of unpacking later.
> > u_commit=$(
> > - untracked_files | (
> > + untracked_files "$@" | (
>
> The above (and all other uses of "$@" was exactly what I had in mind
> when I gave you the "can we leave the '$@' the user gave us as-is?"
> comment during the review of the previous round.
>
> Looks much nicer.
>
> > +test_expect_success 'stash push with $IFS character' '
> > + >"foo bar" &&
>
> IIRC, a pathname with HT in it cannot be tested on MINGW. For the
> purpose of this test, I think it is sufficient to use SP instead of
> HT here (and matching change below in the expectation, of course).
Will change.
> > + >foo &&
> > + >bar &&
> > + git add foo* &&
> > + git stash push --include-untracked -- foo* &&
>
> While it is good that you are testing "foo*", you would also want
> another test to cover a pathspec with $IFS in it. That is the case
> the code in the previous round had problems with. Perhaps try
> something like
>
> >foo && >bar && >"foo bar" && git add . &&
> echo modify foo >foo &&
> echo modify bar >bar &&
> echo modify "foo bar" >"foo bar" &&
> git stash push -- "foo b*"
>
> which should result in the changes to "foo bar" in the resulting
> stash, while changes to "foo" and "bar" are not (and left in the
> working tree). And make sure that is what happens? I think with
> the code from the previous round, the above would instead stash
> changes to "foo" and "bar" without catching changes to "foo bar",
> because the single pathspec "foo b*" would have been mistakenly
> split into two, i.e. "foo" and "b*", and failed to match "foo bar"
> while matching the other two.
Thanks, I'll add a test for that.
^ permalink raw reply
* [RFC PATCH] show decorations at the end of the line
From: Linus Torvalds @ 2017-02-11 18:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
So I use "--show-decorations" all the time because I find it very useful
to see where the origin branch is, where tags are etc. In fact, my global
git config file has
[log]
decorate = auto
in it, so that I don't have to type it out all the time when I just do my
usual 'git log". It's lovely.
However, it does make one particular case uglier: with commit decorations,
the "oneline" commit format ends up being not very pretty:
[torvalds@i7 git]$ git log --oneline -10
3f07dac29 (HEAD -> master) pathspec: don't error out on all-exclusionary pathspec patterns
ca4a562f2 pathspec magic: add '^' as alias for '!'
02555c1b2 ls-remote: add "--diff" option to show only refs that differ
6e3a7b339 (tag: v2.12.0-rc0, origin/master, origin/HEAD) Git 2.12-rc0
fafca0f72 Merge branch 'cw/log-updates-for-all-refs-really'
74dee5cfa Merge branch 'pl/complete-diff-submodule-diff'
36acf4123 Merge branch 'rs/object-id'
ecc486b1f Merge branch 'js/re-running-failed-tests'
4ba6bb2d1 Merge branch 'sb/submodule-update-initial-runs-custom-script'
5348021c6 Merge branch 'sb/submodule-recursive-absorb'
and note how the decoration comes right after the shortened commit hash,
breaking up the alignment of the messages.
The above doesn't show it with the colorization: I also have
[color]
ui=auto
so on my terminal the decoration is also nicely colorized which makes it
much more obvious, it's not as obvious in this message.
The oneline message handling is already pretty special, this makes it even
more special by putting the decorations at the end of the line:
3f07dac29 pathspec: don't error out on all-exclusionary pathspec patterns (HEAD -> master)
ca4a562f2 pathspec magic: add '^' as alias for '!'
02555c1b2 ls-remote: add "--diff" option to show only refs that differ
6e3a7b339 Git 2.12-rc0 (tag: v2.12.0-rc0, origin/master, origin/HEAD)
fafca0f72 Merge branch 'cw/log-updates-for-all-refs-really'
74dee5cfa Merge branch 'pl/complete-diff-submodule-diff'
36acf4123 Merge branch 'rs/object-id'
ecc486b1f Merge branch 'js/re-running-failed-tests'
4ba6bb2d1 Merge branch 'sb/submodule-update-initial-runs-custom-script'
5348021c6 Merge branch 'sb/submodule-recursive-absorb'
which looks a lot better (again, this is all particularly noticeable with
colorization).
NOTE! There's a very special case for "git log --oneline -g" that shows
the reflogs as oneliners, and this does *not* fix that special case. It's
a lot more involved and relies on the exact show_reflog_message()
implementation, so I left the format for that alone, along with a comment
about how it's not at the end of line.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
I've signed off on this, because I think it's an "obvious" improvement,
but I'm putting the "RFC" in the subject line because this is clearly a
subjective thing.
"oneline" really is special: the other commit formats will just put the
commit SHA1 at the end of the line anyway. And with manual formats, the
placement of decorations is also manual, so this doesn't affect that
case.
Comments?
log-tree.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/log-tree.c b/log-tree.c
index 8c2415747..3bf88182e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -622,10 +622,13 @@ void show_log(struct rev_info *opt)
find_unique_abbrev(parent->object.oid.hash,
abbrev_commit));
fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), opt->diffopt.file);
- show_decorations(opt, commit);
if (opt->commit_format == CMIT_FMT_ONELINE) {
+ /* Not at end of line, but.. */
+ if (opt->reflog_info)
+ show_decorations(opt, commit);
putc(' ', opt->diffopt.file);
} else {
+ show_decorations(opt, commit);
putc('\n', opt->diffopt.file);
graph_show_oneline(opt->graph);
}
@@ -716,6 +719,8 @@ void show_log(struct rev_info *opt)
opt->missing_newline = 0;
graph_show_commit_msg(opt->graph, opt->diffopt.file, &msgbuf);
+ if (ctx.fmt == CMIT_FMT_ONELINE)
+ show_decorations(opt, commit);
if (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {
if (!opt->missing_newline)
graph_show_padding(opt->graph);
^ permalink raw reply related
* Re: [RFC PATCH] show decorations at the end of the line
From: Linus Torvalds @ 2017-02-11 18:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <alpine.LFD.2.20.1702110943460.31350@i7.lan>
On Sat, Feb 11, 2017 at 10:02 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I've signed off on this, because I think it's an "obvious" improvement,
> but I'm putting the "RFC" in the subject line because this is clearly a
> subjective thing.
Side note: the one downside of showing the decorations at the end of
the line is that now they are obviously at the end of the line - and
thus likely to be more hidden by things like line truncation.
The default git settings (LESS=FRX) no longer truncate log output
lines, but if you use "-S" or "--chop-long-lines", you will obviously
be missing the end of long lines.
So moving the decorations to the end does obviously have a real UI
impact, apart from just being "prettier".
I just wanted to point that out. I still prefer decorations at ends of
lines (and yes, that's despite the fact that I actually personally use
the traditional git setting of "LESS=FRSX"), but it is perhaps
something that people should be aware of if this patch causes
discussion.
Linus
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Junio C Hamano @ 2017-02-11 18:51 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Jeff Hostetler
In-Reply-To: <31bb0b9f-d498-24b3-57d5-9f34cb8e3914@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 10.02.2017 um 00:41 schrieb Junio C Hamano:
>> ...
>> Nice. Will queue as jh/mingw-openssl-sha1 topic; it is a bit too
>> late for today's integration cycle to be merged to 'next', but let's
>> have this by the end of the week in 'master'.
>
> Please don't rush this through. I didn't have a chance to cross-check
> the patch; it will have to wait for Monday. I would like to address
> Peff's concerns about additional runtime dependencies.
OK. Will mark it as "Will cook in 'next'" for now.
^ permalink raw reply
* Re: [PATCH] cocci: detect useless free(3) calls
From: Junio C Hamano @ 2017-02-11 19:01 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Pranit Bauva
In-Reply-To: <7e10f934-f084-ceb4-00eb-b75cdb01886b@web.de>
René Scharfe <l.s.r@web.de> writes:
> Add a semantic patch for removing checks that cause free(3) to only be
> called with a NULL pointer, as that must be a programming mistake.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> No cases are found in master or next, but 1d263b93 (bisect--helper:
> `bisect_next_check` & bisect_voc shell function in C) introduced four
> of them to pu.
Thanks. This should have been caught by code inspection, but
having automated way to do so is always good.
>
> contrib/coccinelle/free.cocci | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
> index e28213161a..c03ba737e5 100644
> --- a/contrib/coccinelle/free.cocci
> +++ b/contrib/coccinelle/free.cocci
> @@ -3,3 +3,9 @@ expression E;
> @@
> - if (E)
> free(E);
> +
> +@@
> +expression E;
> +@@
> +- if (!E)
> + free(E);
^ permalink raw reply
* Re: [PATCH] cocci: detect useless free(3) calls
From: Lars Schneider @ 2017-02-11 19:31 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano, Pranit Bauva
In-Reply-To: <7e10f934-f084-ceb4-00eb-b75cdb01886b@web.de>
> On 11 Feb 2017, at 14:58, René Scharfe <l.s.r@web.de> wrote:
>
> Add a semantic patch for removing checks that cause free(3) to only be
> called with a NULL pointer, as that must be a programming mistake.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> No cases are found in master or next, but 1d263b93 (bisect--helper:
> `bisect_next_check` & bisect_voc shell function in C) introduced four
> of them to pu.
Hi Rene,
how do you run these checks on the entire Git source?
Do you run each semantic patch file on the source like this?
spatch --sp-file contrib/coccinelle/qsort.cocci --dir /path/to/git/git
...
spatch --sp-file contrib/coccinelle/free.cocci --dir /path/to/git/git
How stable do you consider these checks? Would it make sense to run them
as part of the Travis-CI build [1]?
Thanks,
Lars
[1] https://travis-ci.org/git/git/branches
^ permalink raw reply
* Re: [PATCH] cocci: detect useless free(3) calls
From: René Scharfe @ 2017-02-11 19:50 UTC (permalink / raw)
To: Lars Schneider; +Cc: Git List, Junio C Hamano, Pranit Bauva
In-Reply-To: <5DF3AA3E-90C6-443C-A22C-489CE2C89A51@gmail.com>
Am 11.02.2017 um 20:31 schrieb Lars Schneider:
> how do you run these checks on the entire Git source?
> Do you run each semantic patch file on the source like this?
>
> spatch --sp-file contrib/coccinelle/qsort.cocci --dir /path/to/git/git
> ...
> spatch --sp-file contrib/coccinelle/free.cocci --dir /path/to/git/git
With "make coccicheck", which runs spatch against the items in the make
variable C_SOURCES, for all .cocci files.
> How stable do you consider these checks? Would it make sense to run them
> as part of the Travis-CI build [1]?
There seem to have been problems with older versions[2], but I'm not
aware of other issues.
Having these checks run automatically would be nice because they
require a special tool which I guess not everybody is willing (or able)
to install and because they take multiple minutes.
René
[2] https://public-inbox.org/git/014ef44e-9dd8-40b3-a3ec-b483f938ee02@web.de/
^ permalink raw reply
* [PATCH] rm: reuse strbuf for all remove_dir_recursively() calls, again
From: René Scharfe @ 2017-02-11 19:51 UTC (permalink / raw)
To: Git List, Stefan Beller; +Cc: Junio C Hamano
Don't throw the memory allocated for remove_dir_recursively() away after
a single call, use it for the other entries as well instead.
This change was done before in deb8e15a (rm: reuse strbuf for all
remove_dir_recursively() calls), but was reverted as a side-effect of
55856a35 (rm: absorb a submodules git dir before deletion). Reinstate
the optimization.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Was deb8e15a a rebase victim?
builtin/rm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 452170a3ab..fb79dcab18 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -360,15 +360,14 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
*/
if (!index_only) {
int removed = 0, gitmodules_modified = 0;
+ struct strbuf buf = STRBUF_INIT;
for (i = 0; i < list.nr; i++) {
const char *path = list.entry[i].name;
if (list.entry[i].is_submodule) {
- struct strbuf buf = STRBUF_INIT;
-
+ strbuf_reset(&buf);
strbuf_addstr(&buf, path);
if (remove_dir_recursively(&buf, 0))
die(_("could not remove '%s'"), path);
- strbuf_release(&buf);
removed = 1;
if (!remove_path_from_gitmodules(path))
@@ -382,6 +381,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
if (!removed)
die_errno("git rm: '%s'", path);
}
+ strbuf_release(&buf);
if (gitmodules_modified)
stage_updated_gitmodules();
}
--
2.11.1
^ permalink raw reply related
* Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
From: Junio C Hamano @ 2017-02-11 21:08 UTC (permalink / raw)
To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <20170211075254.GA16053@ubuntu-512mb-blr1-01.localdomain>
Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> On Fri, Feb 10, 2017 at 03:35:47PM -0800, Junio C Hamano wrote:
>
>> I am wondering if writing it like the following is easier to
>> understand. I had a hard time figuring out what you are trying to
>> do, partly because "args" is quite a misnomer---implying "how many
>> arguments did we see" that is similar to opts that does mean "how
>> many options did handle_revision_opts() see?"
>
> Um, okay, I see that "args" is very confusing. Would it help if this variable
> was called "arg_not_rev"?
Not really. If we absolutely need to have one variable that is
meant to escape the "if it begins with a dash" block and to affect
what comes next, I think the variable should mean "we know we saw a
revision and you do not have to call it again". IOW the code that
needs to do "handle_rev_arg_called && arg_not_rev" is just being
silly. At that point in the codeflow, I do not see why the code
needs to take two bits of information and combine them; the one that
sets these two variables should have done the work for it.
And that would make the if statement slightly easier to read
compared to the original. I am however not suggesting to do that;
read on.
> Because the value that is returned from
> handle_revision_arg is 1 when it is not a revision, and 0 when it is a
> revision.
The function follows the convention to return 0 for success, -1 for
error/unexpected, by the way.
> Um, I am sorry, but I feel that decrementing left, and incrementing it again is
> also confusing.
Yes, but it is no more confusing than your original "left--".
If we want to make the flow of logic easier to follow, we need to
step back and view what the codepath _wants_ to do at the higher
level, which is:
* If it is an option known to us, handle it and go to the next arg.
* If it is an option that we do not understand, stuff it in
argv[left++] and go to the next arg.
* If it is a rev, handle it, and note that fact in got_rev_arg.
* If it is not a rev and we haven't seen dashdash, verify that it
and everything that follows it are pathnames (which is an inexact
but a cheap way to avoid ambiguity), make all them the prune_data
and conclude.
Because the second step currently is implemented by calling
handle_opt(), which not just tells if it is an option we understand
or not, but also mucks with argv[left++], you need to undo it once
you start making it possible for a valid "rev" to begin with a dash.
That is what your left-- was, and that is what "decrement and then
increment when it turns out it was an unknown option after all" is.
The first step to a saner flow _could_ be to stop passing the unkv
and unkc to handle_revision_opt() and instead make the caller
responsible for doing that. That would express what your patch
wanted to do in the most natural way, i.e.
* If it is an option known to us, handle it and go to the next arg.
* If it is a rev, handle it, and note that fact in got_rev_arg
(this change of order enables you to allow a rev that begins with
a dash, which would have been misrecognised as a possible unknown
option).
* If it looks like an option (i.e. "begins with a dash"), then we
already know that it is not something we understand, because the
first step would have caught it already. Stuff it in
argv[left++] and go to the next arg.
* If it is not a rev and we haven't seen dashdash, verify that it
and everything that follows it are pathnames (which is an inexact
but a cheap way to avoid ambiguity), make all them the prune_data
and conclude.
Such a change to handle_revision_opt() unfortunately affects other
callers of the function, so it may not be worth it, but I think
"decrement and then increment, because this codepath wants to check
to see something that may ordinarily be clasified as an unknown
option if it is a rev" is an ugly workaround, just like your left--
was. But I think the resulting code flow is much closer to the
above ideal.
^ permalink raw reply
* Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
From: Junio C Hamano @ 2017-02-11 23:40 UTC (permalink / raw)
To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <xmqqefz4h1vq.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Such a change to handle_revision_opt() unfortunately affects other
> callers of the function, so it may not be worth it, and I think
> "decrement and then increment, because this codepath wants to check
> to see something that may ordinarily be clasified as an unknown
> option if it is a rev" is an ugly workaround, just like your left--
> was. But I think the resulting code flow is much closer to the
> above ideal.
Having re-analysed the codepath like so, I realize that the new
variable I introduced was misnamed. Its purpose is to let the
"if arg begins with dash, do this" block communicate that what the
later part of the code is told to inspect in "arg" may be an option
that we do not recognise. So I shouldn't have called it maybe_rev;
the message from the former to the latter is "this may be an unknown
option" and I should have called it "maybe_unknown_opt".
^ permalink raw reply
* [RFC] send-email: avoid duplicate In-Reply-To and References headers
From: Eric Wong @ 2017-02-12 0:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Vasco Almeida
When parsing an mbox, it is possible to get existing In-Reply-To
and References headers blindly appended into the headers of
message we generate. This is probably the wrong thing to do
and we should prioritize what was given in the command-line,
cover letter, and previously-sent messages.
One example I've noticed in the wild was:
https://public-inbox.org/git/20161111124541.8216-17-vascomalmeida@sapo.pt/raw
---
I'm not completely sure this is what Vasco was doing in that
message, so it's an RFC for now...
git-send-email.perl | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 068d60b3e6..5ab3d8585c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1543,7 +1543,13 @@ foreach my $t (@files) {
elsif (!/^Date:\s/i && /^[-A-Za-z]+:\s+\S/) {
push @xh, $_;
}
-
+ elsif (/^(?:References|In-Reply-To):/i) {
+ if (defined $initial_reply_to || $thread) {
+ warn
+"Ignoring $_ header in mbox body since it conflicts with\n
+--in-reply-to and --thread switches\n"
+ }
+ }
} else {
# In the traditional
# "send lots of email" format,
--
EW
^ permalink raw reply related
* Re: [RFC] send-email: avoid duplicate In-Reply-To and References headers
From: Junio C Hamano @ 2017-02-12 1:41 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Jeff King, Vasco Almeida
In-Reply-To: <20170212003432.GA19519@starla>
Eric Wong <e@80x24.org> writes:
> When parsing an mbox, it is possible to get existing In-Reply-To
> and References headers blindly appended into the headers of
> message we generate. This is probably the wrong thing to do
> and we should prioritize what was given in the command-line,
> cover letter, and previously-sent messages.
>
> One example I've noticed in the wild was:
>
> https://public-inbox.org/git/20161111124541.8216-17-vascomalmeida@sapo.pt/raw
> ---
> I'm not completely sure this is what Vasco was doing in that
> message, so it's an RFC for now...
I think it is sensibleto give priority to the --in-reply-to option
given from the command line over the in-file one. I am not sure if
we want to drop references, though. Wouldn't it make more sense to
just add what we got from the command line to what we read from the
file? I dunno.
> git-send-email.perl | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 068d60b3e6..5ab3d8585c 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1543,7 +1543,13 @@ foreach my $t (@files) {
> elsif (!/^Date:\s/i && /^[-A-Za-z]+:\s+\S/) {
> push @xh, $_;
> }
> -
> + elsif (/^(?:References|In-Reply-To):/i) {
> + if (defined $initial_reply_to || $thread) {
> + warn
> +"Ignoring $_ header in mbox body since it conflicts with\n
> +--in-reply-to and --thread switches\n"
> + }
> + }
> } else {
> # In the traditional
> # "send lots of email" format,
^ permalink raw reply
* Re: [RFC] send-email: avoid duplicate In-Reply-To and References headers
From: Eric Wong @ 2017-02-12 2:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Vasco Almeida
In-Reply-To: <xmqq60kggp8r.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
> > When parsing an mbox, it is possible to get existing In-Reply-To
> > and References headers blindly appended into the headers of
> > message we generate. This is probably the wrong thing to do
> > and we should prioritize what was given in the command-line,
> > cover letter, and previously-sent messages.
> >
> > One example I've noticed in the wild was:
> >
> > https://public-inbox.org/git/20161111124541.8216-17-vascomalmeida@sapo.pt/raw
> > ---
> > I'm not completely sure this is what Vasco was doing in that
> > message, so it's an RFC for now...
>
> I think it is sensibleto give priority to the --in-reply-to option
> given from the command line over the in-file one. I am not sure if
> we want to drop references, though. Wouldn't it make more sense to
> just add what we got from the command line to what we read from the
> file? I dunno.
You're right, existing References in the bodies should probably
be prepended to current ones, as their order should be
oldest-to-newest.
I'll wait on comments a bit and work on a better version w/ tests
next week.
^ permalink raw reply
* Re: [RFC] send-email: avoid duplicate In-Reply-To and References headers
From: Junio C Hamano @ 2017-02-12 2:51 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Jeff King, Vasco Almeida
In-Reply-To: <20170212021208.GA16358@starla>
Eric Wong <e@80x24.org> writes:
> Junio C Hamano <gitster@pobox.com> wrote:
>>
>> I think it is sensibleto give priority to the --in-reply-to option
>> given from the command line over the in-file one. I am not sure if
>> we want to drop references, though. Wouldn't it make more sense to
>> just add what we got from the command line to what we read from the
>> file? I dunno.
>
> You're right, existing References in the bodies should probably
> be prepended to current ones, as their order should be
> oldest-to-newest.
If you were to go that way, it may also make sense to demote the
original In-Reply-To you find in the file to an entry in the
References list, if it does not already appear there.
I didn't check the RFCs, but I think your original motivation is to
ensure that there is not more than one messages to which the current
message is replying to, iow, to ensure that there is only one message
that is In-Reply-To, so appending would not be a good solution there.
^ permalink raw reply
* Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"
From: Matthieu Moy @ 2017-02-12 9:48 UTC (permalink / raw)
To: Siddharth Kannan; +Cc: git, gitster, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <1486752926-12020-3-git-send-email-kannan.siddharth12@gmail.com>
Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> sha1_name.c | 5 ++++
> t/t4214-log-shorthand.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 78 insertions(+)
> create mode 100755 t/t4214-log-shorthand.sh
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 73a915f..d774e46 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
> if (!ret)
> return 0;
>
> + if (!strcmp(name, "-")) {
> + name = "@{-1}";
> + len = 5;
> + }
One drawback of this approach is that further error messages will be
given from the "@{-1}" string that the user never typed.
After you do that, the existing "turn - into @{-1}" pieces of code
become useless and you should remove it (probably in a further patch).
There are at least:
$ git grep -n -A1 'strcmp.*"-"' | grep -B 1 '@\{1\}'
builtin/checkout.c:975: if (!strcmp(arg, "-"))
builtin/checkout.c-976- arg = "@{-1}";
--
builtin/merge.c:1231: } else if (argc == 1 && !strcmp(argv[0], "-")) {
builtin/merge.c-1232- argv[0] = "@{-1}";
--
builtin/revert.c:158: if (!strcmp(argv[1], "-"))
builtin/revert.c-159- argv[1] = "@{-1}";
--
builtin/worktree.c:344: if (!strcmp(branch, "-"))
builtin/worktree.c-345- branch = "@{-1}";
In the final version, obviously the same "refactoring" (specific
command -> git-wide) should be done for documentation (it should be in
this patch to avoid letting not-up-to-date documentation even for a
single commit).
> diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh
> new file mode 100755
> index 0000000..dec966c
> --- /dev/null
> +++ b/t/t4214-log-shorthand.sh
> @@ -0,0 +1,73 @@
> +#!/bin/sh
> +
> +test_description='log can show previous branch using shorthand - for @{-1}'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> + echo hello >world &&
> + git add world &&
> + git commit -m initial &&
> + echo "hello second time" >>world &&
> + git add world &&
> + git commit -m second &&
> + echo "hello other file" >>planet &&
> + git add planet &&
> + git commit -m third &&
> + echo "hello yet another file" >>city &&
> + git add city &&
> + git commit -m fourth
> +'
You may use test_commit to save a few lines of code.
> +test_expect_success '"log -" should work' '
> + git checkout -b testing-1 master^ &&
> + git checkout -b testing-2 master~2 &&
> + git checkout master &&
> +
> + git log testing-2 >expect &&
> + git log - >actual &&
> + test_cmp expect actual
> +'
I'd have split this into a "setup branches" and a '"log -" should work'
test (to actually see where "setup branches" happen in the output, and
to allow running the setup step separately if needed). Not terribly
important.
> +test_expect_success 'symmetric revision range should work when one end is left empty' '
> + git checkout testing-2 &&
> + git checkout master &&
> + git log ...@{-1} > expect.first_empty &&
> + git log @{-1}... > expect.last_empty &&
> + git log ...- > actual.first_empty &&
> + git log -... > actual.last_empty &&
Nitpick: we stick the > and the filename (as you did in most places
already).
It may be worth adding tests for more cases like
* Check what happens with suffixes, i.e. -^, -@{yesterday} and -~.
* -..- -> to make sure you handle the presence of two - properly.
* multiple separate arguments to make sure you handle them all, e.g.
"git log - -", "git log HEAD -", "git log - HEAD".
The last two may be overkill, but the first one is probably important.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"
From: Siddharth Kannan @ 2017-02-12 10:42 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, gitster, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <vpqbmu768on.fsf@anie.imag.fr>
Hey Matthieu,
On Sun, Feb 12, 2017 at 10:48:56AM +0100, Matthieu Moy wrote:
> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>
> > sha1_name.c | 5 ++++
> > t/t4214-log-shorthand.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 78 insertions(+)
> > create mode 100755 t/t4214-log-shorthand.sh
> >
> > diff --git a/sha1_name.c b/sha1_name.c
> > index 73a915f..d774e46 100644
> > --- a/sha1_name.c
> > +++ b/sha1_name.c
> > @@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
> > if (!ret)
> > return 0;
> >
> > + if (!strcmp(name, "-")) {
> > + name = "@{-1}";
> > + len = 5;
> > + }
>
> After you do that, the existing "turn - into @{-1}" pieces of code
> become useless and you should remove it (probably in a further patch).
Yeah, this is currently also implemented in checkout, apart from the
grepped list that you have supplied here. I will find all the
instances, and ensure that they work, and remove them. (This will
require some more digging into the codepath the commands, to ensure
that get_sha1_1 is called somewhere down the line)
>
> > diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh
> > ...
> > +test_expect_success 'setup' '
> > + echo hello >world &&
> > + git add world &&
> > + git commit -m initial &&
> > + echo "hello second time" >>world &&
> > ...
>
> You may use test_commit to save a few lines of code.
Oh, yeah! I will use that. I need to work on improving the tests, as
well as adding the documentation.
>
> > +test_expect_success 'symmetric revision range should work when one end is left empty' '
> > + git checkout testing-2 &&
> > + git checkout master &&
> > + git log ...@{-1} > expect.first_empty &&
> > + git log @{-1}... > expect.last_empty &&
> > + git log ...- > actual.first_empty &&
> > + git log -... > actual.last_empty &&
>
> Nitpick: we stick the > and the filename (as you did in most places
> already).
Sorry, slipped my mind!
>
> It may be worth adding tests for more cases like
>
> * Check what happens with suffixes, i.e. -^, -@{yesterday} and -~.
These do not work right now. The first and last cases here are handled
by peel_onion, if I remember correctly. I have to find out why exactly
these are not working. Thanks for mentioning this!
>
> * -..- -> to make sure you handle the presence of two - properly.
>
> * multiple separate arguments to make sure you handle them all, e.g.
> "git log - -", "git log HEAD -", "git log - HEAD".
Yeah, will add these tests.
>
> The last two may be overkill, but the first one is probably important.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
--
Regards,
Siddharth Kannan.
^ permalink raw reply
* Bug in 'git describe' if I have two tags on the same commit.
From: Istvan Pato @ 2017-02-12 12:15 UTC (permalink / raw)
To: git
I didn't get back the latest tag by 'git describe --tags --always' if
I have two tags on the same commit.
// repository ppa:git-core/ppa
(master)⚡ % cat /etc/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=16.04
DISTRIB_CODENAME=xenial
DISTRIB_DESCRIPTION="Ubuntu 16.04.2 LTS"
(master)⚡ % git --version
git version 2.11.0
(master) [1] % git show-ref --tag
76c634390... refs/tags/1.0.0
b77c7cd17... refs/tags/1.1.0
b77c7cd17... refs/tags/1.2.0
(master) % git describe --tags --always
1.1.0-1-ge9e9ced
### Expected: 1.2.0
References:
https://www.kernel.org/pub/software/scm/git/docs/RelNotes-1.7.1.1.txt
* "git describe" did not tie-break tags that point at the same commit
correctly; newer ones are preferred by paying attention to the
tagger date now.
http://stackoverflow.com/questions/8089002/git-describe-with-two-tags-on-the-same-commit
Thanks,
Istvan Pato
^ permalink raw reply
* Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
From: Siddharth Kannan @ 2017-02-12 12:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <xmqqefz4h1vq.fsf@gitster.mtv.corp.google.com>
On Sat, Feb 11, 2017 at 01:08:09PM -0800, Junio C Hamano wrote:
> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>
> > Um, I am sorry, but I feel that decrementing left, and incrementing it again is
> > also confusing.
>
> Yes, but it is no more confusing than your original "left--".
> ...
>
> * If it is an option known to us, handle it and go to the next arg.
>
> * If it is an option that we do not understand, stuff it in
> argv[left++] and go to the next arg.
>
> Because the second step currently is implemented by calling
> handle_opt(), which not just tells if it is an option we understand
> or not, but also mucks with argv[left++], you need to undo it once
> you start making it possible for a valid "rev" to begin with a dash.
> That is what your left-- was, and that is what "decrement and then
> increment when it turns out it was an unknown option after all" is.
So, our problem here is that the function handle_revision_opt is opaquely also
incrementing "left", which we need to decrement somehow.
Or: we could change the flow of the code so that this incrementing
will happen only when we have decided that the argument is not a
revision.
>
> * If it is an option known to us, handle it and go to the next arg.
>
> * If it is a rev, handle it, and note that fact in got_rev_arg
> (this change of order enables you to allow a rev that begins with
> a dash, which would have been misrecognised as a possible unknown
> option).
>
> * If it looks like an option (i.e. "begins with a dash"), then we
> already know that it is not something we understand, because the
> first step would have caught it already. Stuff it in
> argv[left++] and go to the next arg.
>
> * If it is not a rev and we haven't seen dashdash, verify that it
> and everything that follows it are pathnames (which is an inexact
> but a cheap way to avoid ambiguity), make all them the prune_data
> and conclude.
This "changing the order" gave me the idea to change the flow. I tried to
implement the above steps without touching the function handle_revision_opt. By
inserting the handle_revision_arg call just before calling handle_revision_opt.
The decrementing then incrementing or "left--" things have now been removed.
(But there is still one thing which doesn't look good)
diff --git a/revision.c b/revision.c
index b37dbec..8c0acea 100644
--- a/revision.c
+++ b/revision.c
@@ -2203,11 +2203,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
if (seen_dashdash)
revarg_opt |= REVARG_CANNOT_BE_FILENAME;
read_from_stdin = 0;
+
for (left = i = 1; i < argc; i++) {
const char *arg = argv[i];
+ int opts;
if (*arg == '-') {
- int opts;
-
opts = handle_revision_pseudo_opt(submodule,
revs, argc - i, argv + i,
&flags);
@@ -2226,7 +2226,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
read_revisions_from_stdin(revs, &prune_data);
continue;
}
+ }
+ if (!handle_revision_arg(arg, revs, flags, revarg_opt))
+ got_rev_arg = 1;
+ else if (*arg == '-') {
opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv);
if (opts > 0) {
i += opts - 1;
@@ -2234,11 +2238,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (opts < 0)
exit(128);
- continue;
- }
-
-
- if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+ } else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
@@ -2255,8 +2255,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
append_prune_data(&prune_data, argv + i);
break;
}
- else
- got_rev_arg = 1;
}
if (prune_data.nr) {
The "if (*arg =='-')" line is repeated. On analysing the resulting
revision.c:setup_revisions function, I feel that the codepath is still as
easily followable as it was earlier, and there is definitely no confusion
because of a mysterious decrement. Also, the repeated condition doesn't make it
any harder (it looks like a useful check because we already know that every
option would start with a "-"). But that's only my opinion, and you definitely
know better.
now the flow is very close to the ideal flow that we prefer:
1. If it is a pseudo_opt or --stdin, handle and go to the next arg
2. If it is a revision, note that in "got_rev_arg", and go to the next arg
3. If it starts with a "-" and is a known option, handle and go to the next arg
4. If it is none of {revision, known-option} and we haven't seen dashdash,
verify that it and everything that follows it are pathnames (which is an
inexact but a cheap way to avoid ambiguity), make all them the prune_data and
conclude.
> But I think the resulting code flow is much closer to the
> above ideal.
(about Junio's version of the patch): Yes, I agree with you on this. It's like
the ideal, but the argv has already been populated, so the only remaining step
is "left++".
>
> Such a change to handle_revision_opt() unfortunately affects other
> callers of the function, so it may not be worth it.
handle_revision_opt is called once apart from within setup_revisions,
from within revision.c:parse_revision_opt.
If this version is not acceptable, we should either revert back to your version
of the patch with the fixed variable name OR consider re-writing
handle_revision_opt, as per your suggested flow. Note that this will put the
code for "Stuff it in argv[left++]" in every caller.
Thank you for the time you have spent on analysing each version of the patch!
--
Best Regards,
Siddharth Kannan.
^ permalink raw reply related
* Git status performance on PS (command prompt)
From: Mark Gaiser @ 2017-02-12 15:53 UTC (permalink / raw)
To: git
Hi,
I'm using ZSH with some fancy prompt. In that prompt is also a quick
git status overview (some symbols indicating if the branch is up to
date, if there are changes, etc..
The commands that are being executed to fetch the information:
For the file status:
git status --porcelain
For the repository status:
git status --porcelain -b
On small repositories (or even medium sized ones) this is fast, no
problems there.
But on larger repositories this is notably slow (i'm taking QtBase as
an example now, but the same is true for much of the Qt repositories,
or chromium or even the linux kernel itself). It's no problem if it's
slow when you do "git status" on the command line. That can be
expected to take a little while on large repositories. But in zsh
prompts the call to git isn't asynchronous [1] so any slowdown will be
noticed as the prompt simply doesn't completely show till after the
command.
I did a bit of profiling in git to figure out where this slowdown comes from.
Callgrind tells me that "read_directory_recursive" takes up ~62% of the time.
Within that call the function "last_exclude_matching_from_list" takes
up 49% of the time it takes to run "git status --porcelain -b".
I don't really know how this code is supposed to work (i'm a git user,
not a git developer), so i hope someone might be able to investigate
this further. I can however apply patches to git locally and help out
with testing.
Also, is there perhaps a command out there that might be better suited
for the git status i want to show in the prompt? What i have now is
merely from one of the oh-my-zsh themes.
Best regards,
Mark
p.s. please keep me in cc, i'm not subscribed to this list.
[1] Most prompts don't have async support, but there are prompts that
do provide this: https://github.com/sindresorhus/pure I guess that
would be a better solution for me in the short term. Still, having the
status being made faster would be beneficial.
^ permalink raw reply
* Confusing git messages when disk is full.
From: Jáchym Barvínek @ 2017-02-12 16:37 UTC (permalink / raw)
To: git
Hello, I would like to report what I consider a bug in git, I hope I'm
doing it the right way.
I was trying to run `git pull` in my repository and got the following
error: "git pull
Your configuration specifies to merge with the ref 'refs/heads/master'
from the remote, but no such ref was fetched."
Which was very confusing to me, I found some answers to what might be the cause
but none was the right one. The actual cause was that the filesystem
had no more free space.
When I cleaned the space, `git pull` then gave the expected answer
("Already up-to-date.").
I think the message is confusing and git should be able to report to
the user that the cause
is full disk.
Regards, Jáchym Barvínek
^ permalink raw reply
* Re: Git status performance on PS (command prompt)
From: brian m. carlson @ 2017-02-12 16:46 UTC (permalink / raw)
To: Mark Gaiser; +Cc: git
In-Reply-To: <CAPd6JnGC3yeDo_sB+H+UKvh-PbiC2qySC=EbaRNLbkkwYkJM4A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3011 bytes --]
On Sun, Feb 12, 2017 at 04:53:47PM +0100, Mark Gaiser wrote:
> Hi,
>
> I'm using ZSH with some fancy prompt. In that prompt is also a quick
> git status overview (some symbols indicating if the branch is up to
> date, if there are changes, etc..
>
> The commands that are being executed to fetch the information:
>
> For the file status:
> git status --porcelain
>
> For the repository status:
> git status --porcelain -b
So your prompt is running git status twice? Wouldn't it make more sense
to just run it once and do a head -n 1 to filter out the branch when you
need it? git symbolic-ref HEAD might work as well.
> On small repositories (or even medium sized ones) this is fast, no
> problems there.
> But on larger repositories this is notably slow (i'm taking QtBase as
> an example now, but the same is true for much of the Qt repositories,
> or chromium or even the linux kernel itself). It's no problem if it's
> slow when you do "git status" on the command line. That can be
> expected to take a little while on large repositories. But in zsh
> prompts the call to git isn't asynchronous [1] so any slowdown will be
> noticed as the prompt simply doesn't completely show till after the
> command.
>
> I did a bit of profiling in git to figure out where this slowdown comes from.
> Callgrind tells me that "read_directory_recursive" takes up ~62% of the time.
> Within that call the function "last_exclude_matching_from_list" takes
> up 49% of the time it takes to run "git status --porcelain -b".
This isn't entirely surprising. When git status runs, it checks all the
files to see if the stat information is changed, and if so, updates the
status. last_exclude_matching_from_list determines if the files are
ignored, which is also necessary to ensure that only the proper files
are listed.
That doesn't mean that there couldn't be some improvements there,
though.
> I don't really know how this code is supposed to work (i'm a git user,
> not a git developer), so i hope someone might be able to investigate
> this further. I can however apply patches to git locally and help out
> with testing.
>
> Also, is there perhaps a command out there that might be better suited
> for the git status i want to show in the prompt? What i have now is
> merely from one of the oh-my-zsh themes.
I typically use the vcs_info support built into zsh[0], although that
can be very slow on my phone. It performs adequately on most
repositories on my laptop and server machines, though, including on the
Linux kernel repo and our 2.4 and 9 GB repos at work.
I don't know that it has all the functionality that you need, as git
status provides finer-grained data, but it might.
[0] Example at https://github.com/bk2204/homedir/blob/master/.zsh/prompt_bmc_setup
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2017, #03; Fri, 10)
From: Vegard Nossum @ 2017-02-12 18:32 UTC (permalink / raw)
To: Junio C Hamano, René Scharfe; +Cc: git
In-Reply-To: <xmqq8tpdifoa.fsf@gitster.mtv.corp.google.com>
On 11/02/2017 04:12, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
>> Am 10.02.2017 um 23:24 schrieb Junio C Hamano:
>>> * vn/xdiff-func-context (2017-01-15) 1 commit
>>> - xdiff -W: relax end-of-file function detection
>>>
>>> "git diff -W" has been taught to handle the case where a new
>>> function is added at the end of the file better.
>>>
>>> Will hold.
>>> Discussion on an follow-up change to go back from the line that
>>> matches the funcline to show comments before the function
>>> definition has not resulted in an actionable conclusion.
>>
>> This one is a bug fix and can be merged already IMHO.
>
> Absolutely. I was just waiting if the follow-up discussion would
> easily and quickly lead to another patch, forgot about what exactly
> I was waiting for (i.e. the gravity of not having the follow-up),
> and have left it in "Will hold" status forever.
I said I would resubmit the patches with more config options and more
command-line arguments (to avoid potentially breaking backwards
compatibility), but IIRC the response seemed to be "preceding blank line
heuristic is good enough" and "why bother", so I ended up not not
resubmitting anything.
>
> Let's merge it to 'next' and then decide if we want to also merge it
> to 'master' before the final. The above step alone is a lot less
> contriversial and tricky bugfix.
Thanks,
Vegard
^ permalink raw reply
* [PATCH 0/3] prepare for a rev/range that begins with a dash
From: Junio C Hamano @ 2017-02-12 18:41 UTC (permalink / raw)
To: git; +Cc: Siddharth Kannan
In-Reply-To: <xmqqa89sguu4.fsf@gitster.mtv.corp.google.com>
It turns out that telling handle_revision_opt() not to molest argv[left++]
does not have heavy fallout.
Junio C Hamano (3):
handle_revision_opt(): do not update argv[left++] with an unknown arg
setup_revisions(): swap if/else bodies to make the next step more readable
setup_revisions(): allow a rev that begins with a dash
revision.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
--
2.12.0-rc1-212-ga9adfb24fa
^ permalink raw reply
* [PATCH 1/3] handle_revision_opt(): do not update argv[left++] with an unknown arg
From: Junio C Hamano @ 2017-02-12 18:41 UTC (permalink / raw)
To: git; +Cc: Siddharth Kannan
In-Reply-To: <20170212184132.12375-1-gitster@pobox.com>
In future steps, we will make it possible for a rev or a revision
range (i.e. what is understood by handle_revision_arg() helper) to
begin with a dash. The setup_revisions() function however currently
considers anything that begins with a dash to be:
- an option it itself understands and handles (some take effect by
setting fields in the revision structure, some others are left
in the argv[left++] to be handled in later steps);
- an option handle_revision_opt() understands and tells us to skip;
- an option handle_revision_opt() found to be incorrect; or
- an option handle_revision_opt() did not understand, which is
stuffed in argv[left++].
and does not give a chance to handle_revision_arg() to inspect it.
The handle_revision_opt() function returns a positive count, a
negative count or zero to allow the caller to tell the latter three
cases apart. A rev that begins with a dash would be thrown into the
last category.
Teach handle_revision_opt() not to touch argv[left++] in the last
case. Because the other one among the two callers of this function
immediately errors out with the usage string when it returns zero
(i.e. the last case above), there is no negative effect to that
caller.
In setup_revisions(), which is the other caller of this function,
we need to stuff the unknown arg to argv[left++] in this case, to
preserve the current behaviour.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
revision.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index b37dbec378..4f46b8ba81 100644
--- a/revision.c
+++ b/revision.c
@@ -2016,8 +2016,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->ignore_missing = 1;
} else {
int opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);
- if (!opts)
- unkv[(*unkc)++] = arg;
return opts;
}
if (revs->graph && revs->track_linear)
@@ -2234,6 +2232,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (opts < 0)
exit(128);
+ /* arg looks like an opt but something we do not recognise. */
+ argv[left++] = arg;
continue;
}
--
2.12.0-rc1-212-ga9adfb24fa
^ permalink raw reply related
* [PATCH 2/3] setup_revisions(): swap if/else bodies to make the next step more readable
From: Junio C Hamano @ 2017-02-12 18:41 UTC (permalink / raw)
To: git; +Cc: Siddharth Kannan
In-Reply-To: <20170212184132.12375-1-gitster@pobox.com>
Swap the condition and bodies of an "if (A) do_A else do_B" in
setup_revisions() to "if (!A) do_B else do A", to make the change in
the the next step easier to read.
No behaviour change is intended in this step.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
revision.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/revision.c b/revision.c
index 4f46b8ba81..eccf1ab695 100644
--- a/revision.c
+++ b/revision.c
@@ -2237,8 +2237,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
continue;
}
-
- if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+ if (!handle_revision_arg(arg, revs, flags, revarg_opt)) {
+ got_rev_arg = 1;
+ } else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
@@ -2255,8 +2256,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
append_prune_data(&prune_data, argv + i);
break;
}
- else
- got_rev_arg = 1;
}
if (prune_data.nr) {
--
2.12.0-rc1-212-ga9adfb24fa
^ permalink raw reply related
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