* 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
* 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
* 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
* 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 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
* 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: [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: [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 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
* [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: [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
* 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
* [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] 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
* 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: 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] 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: [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
* [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: [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
* [RFH] Request for Git Merge 2017 impressions for Git Rev News
From: Jakub Narębski @ 2017-02-11 16:33 UTC (permalink / raw)
To: git
Hello,
Git Rev News #24 is planned to be released on February 15. It is meant
to cover what happened during the month of January 2017 (and earely
February 2017) and the Git Merge 2017 conference that happened on
February 2nd and 3rd 2017.
A draft of a new Git Rev News edition is available here:
https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-24.md
I would like to ask everyone who attended the conference (and the
GitMerge 2017 Contributors’s Summit day before it), or watched it live
at http://git-merge.com/watch to write his or her impressions.
You can contribute either by replying to this email, or by editing the
above page on GitHub and sending a pull request, or by commenting on
the following GitHub issue about Git Rev News 24:
https://github.com/git/git.github.io/issues/221
If you prefer to post on your own blog (or if you have did it
already), please send an URL.
P.S. I wonder if there should be not a separate section on
https://git.github.io/ about recollection from various Git-related
events, with Git Merge 2017 as the first one. This way we can wait
for later response, and incorporate videos and slides from events, as
they begin to be available.
P.P.S. Please distribute this information more widely.
Thanks in advance,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v3 4/5] stash: introduce new format create
From: Thomas Gummerer @ 2017-02-11 14:51 UTC (permalink / raw)
To: Jeff King
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170206155606.xgkmhg656vuc6uki@sigill.intra.peff.net>
On 02/06, Jeff King wrote:
> On Sun, Feb 05, 2017 at 08:26:41PM +0000, Thomas Gummerer wrote:
>
> > git stash create currently supports a positional argument for adding a
> > message. This is not quite in line with how git commands usually take
> > comments (using a -m flag).
> >
> > Add a new syntax for adding a message to git stash create using a -m
> > flag. This is with the goal of deprecating the old style git stash
> > create with positional arguments.
> >
> > This also adds a -u argument, for untracked files. This is already used
> > internally as another positional argument, but can now be used from the
> > command line.
>
> How do we tell the difference between new-style invocations, and
> old-style ones that look new-style? IOW, I think:
>
> git stash create -m works
>
> currently treats "-m works" as the full message, and it would now become
> just "works".
>
> That may be an acceptable loss for the benefit we are getting. The
> alternative is to make yet another verb for create, as we did with
> save/push). I have a feeling that hardly anybody uses "create", though,
> and it's mostly an implementation detail. So given the obscure nature,
> maybe it's an acceptable level of regression. I dunno.
Right. So I did a quick search on google and github for this, and
there seems one place where git stash create -m is used [1]. From a
quick look it does however not seem like the -m in the stash message
is of any significance there, but rather that the intention was to use
a flag that doesn't exist.
I *think* this regression is acceptable, but I'm happy to introduce
another verb if people think otherwise.
> But either way, it should probably be in the commit message in case
> somebody does have to track it down later.
I'll add something there, thanks!
> -Peff
[1]: https://github.com/Andersbakken/nrdp-scripts/blob/1052fc6781c36c4b227c7dd4838a501341b0862a/bin/git-rstash#L81
^ permalink raw reply
* [PATCH] cocci: detect useless free(3) calls
From: René Scharfe @ 2017-02-11 13:58 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Pranit Bauva
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.
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);
--
2.11.1
^ permalink raw reply related
* Re: [PATCH v3 3/5] stash: add test for the create command line arguments
From: Thomas Gummerer @ 2017-02-11 13:55 UTC (permalink / raw)
To: Jeff King
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170206155012.rb2vydjvlquchwk2@sigill.intra.peff.net>
On 02/06, Jeff King wrote:
> On Sun, Feb 05, 2017 at 08:26:40PM +0000, Thomas Gummerer wrote:
>
> > +test_expect_success 'create stores correct message' '
> > + >foo &&
> > + git add foo &&
> > + STASH_ID=$(git stash create "create test message") &&
> > + echo "On master: create test message" >expect &&
> > + git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
> > + test_cmp expect actual
> > +'
>
> This misses failure exit codes from "git show" because it's on the left
> side of a pipe. Perhaps "git show -s" or "git log -1" would get you the
> same output without needing "head" (and saving a process and the time
> spent generating the diff, as a bonus).
>
> Ditto in the other tests from this patch and later ones.
Good catch, will fix.
> -Peff
^ permalink raw reply
* Re: [PATCH v3 2/5] stash: introduce push verb
From: Thomas Gummerer @ 2017-02-11 13:33 UTC (permalink / raw)
To: Jeff King
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170206154628.v27z5mqhxylz22ba@sigill.intra.peff.net>
[sorry for the late responses, life is keeping me busy]
On 02/06, Jeff King wrote:
> On Sun, Feb 05, 2017 at 08:26:39PM +0000, Thomas Gummerer wrote:
>
> > + -m|--message)
> > + shift
> > + stash_msg=${1?"-m needs an argument"}
> > + ;;
>
> I think this is our first use of the "?" parameter expansion magic. It
> _is_ in POSIX, so it may be fine. We may get complaints from people on
> weird shell variants, though. If that's the only reason to avoid it, I'd
> be inclined to try it and see, as it is much shorter.
>
> OTOH, most of the other usage errors call usage(), and this one doesn't.
> Nor is the error message translatable. Perhaps we should stick to the
> longer form (and add a helper function if necessary to reduce the
> boilerplate).
Yeah I do agree that calling usage is the better option here.
> > +save_stash () {
> > + push_options=
> > + while test $# != 0
> > + do
> > + case "$1" in
> > + --help)
> > + show_help
> > + ;;
> > + --)
> > + shift
> > + break
> > + ;;
> > + -*)
> > + # pass all options through to push_stash
> > + push_options="$push_options $1"
> > + ;;
> > + *)
> > + break
> > + ;;
> > + esac
> > + shift
> > + done
>
> I suspect you could just let "--help" get handled in the pass-through
> case (it generally takes precedence over errors found in other options,
> but I do not see any other parsing errors that could be found by this
> loop). It is not too bad to keep it, though (the important thing is that
> we're not duplicating all of the push_stash options here).
Good point, would be good to get rid of that duplication as well.
> > + if test -z "$stash_msg"
> > + then
> > + push_stash $push_options
> > + else
> > + push_stash $push_options -m "$stash_msg"
> > + fi
>
> Hmm. So $push_options is subject to word-splitting here. That's
> necessary to split the options back apart. It does the wrong thing if
> any of the options had spaces in them. But I don't think there are any
> valid options which do so, and "save" would presumably not grow any new
> options (they would go straight to "push").
>
> So there is a detectable behavior change:
>
> [before]
> $ git stash "--bogus option"
> error: unknown option for 'stash save': --bogus option
> To provide a message, use git stash save -- '--bogus option'
> [etc...]
>
> [after]
> $ git stash "--bogus option"
> error: unknown option for 'stash save': --bogus
> To provide a message, use git stash save -- '--bogus'
>
> but it's probably an acceptable casualty (the "right" way would be to
> shell-quote everything you stuff into $push_options and then eval the
> result when you invoke push_stash).
>
> Likewise, it's usually a mistake to just stick a new option (like "-m")
> after a list of unknown options. But it's OK here because we know we
> removed any "--" or non-option arguments.
>
> -Peff
--
Thomas
^ 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