* Re: [PATCH] stash: disable renames when calling git-diff
From: Junio C Hamano @ 2016-12-06 19:20 UTC (permalink / raw)
To: Jeff King; +Cc: Matthew Patey, git
In-Reply-To: <20161206154120.yyuca35ugyuifpq6@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> If you run:
>>
>> git -c diff.renames=false stash
>>
>> then it works.
>
> And here's a patch to fix it.
Yuck. This obviously has easier to bite more people since we
enabled the renames by default. Thanks for a quick fix.
I wonder why we are using "git diff" here, not the plumbing,
though.
>
> -- >8 --
> Subject: [PATCH] stash: disable renames when calling git-diff
>
> When creating a stash, we need to look at the diff between
> the working tree and HEAD. There's no plumbing command that
> covers this case, so we do so by calling the git-diff
> porcelain. This means we also inherit the usual list of
> porcelain options, which can impact the output in confusing
> ways.
>
> There's at least one known problem: --name-only will not
> mention the source side of a rename, meaning that we will
> fail to stash a deletion in such a case.
>
> The correct solution is to move to an appropriate plumbing
> command. But since there isn't one available, in the short
> term we can plug this particular problem by manually asking
> git-diff not to respect renames.
>
> Reported-by: Matthew Patey <matthew.patey2167@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> There may be other similar problems lurking depending on the various
> config options you have set, but I don't think so. Most of the options
> only affect --patch output.
>
> I do find it interesting that --name-only does not mention the source
> side of a rename. The longer forms like --name-status mention both
> sides. Certainly --name-status does not have any way of saying "this is
> the source side of a rename", but I'd think it would simply mention both
> sides. Anyway, even if that were changed (which would fix this bug), I
> think adding --no-renames is still a good thing to be doing here.
>
> git-stash.sh | 2 +-
> t/t3903-stash.sh | 9 +++++++++
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 4546abaae..40ab2710e 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -115,7 +115,7 @@ create_stash () {
> git read-tree --index-output="$TMPindex" -m $i_tree &&
> GIT_INDEX_FILE="$TMPindex" &&
> export GIT_INDEX_FILE &&
> - git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
> + git diff --name-only -z --no-renames HEAD -- >"$TMP-stagenames" &&
> git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
> git write-tree &&
> rm -f "$TMPindex"
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index e1a6ccc00..2de3e18ce 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -766,4 +766,13 @@ test_expect_success 'stash list --cc shows combined diff' '
> test_cmp expect actual
> '
>
> +test_expect_success 'stash is not confused by partial renames' '
> + mv file renamed &&
> + git add renamed &&
> + git stash &&
> + git stash apply &&
> + test_path_is_file renamed &&
> + test_path_is_missing file
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH] stash: disable renames when calling git-diff
From: Jeff King @ 2016-12-06 19:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthew Patey, git
In-Reply-To: <xmqqh96g27bh.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 06, 2016 at 11:20:18AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> If you run:
> >>
> >> git -c diff.renames=false stash
> >>
> >> then it works.
> >
> > And here's a patch to fix it.
>
> Yuck. This obviously has easier to bite more people since we
> enabled the renames by default. Thanks for a quick fix.
>
> I wonder why we are using "git diff" here, not the plumbing,
> though.
I don't think there's a plumbing command which works for diffing the
working tree directly to a git tree. In the long run, it might be a good
idea to remedy that.
Though I'm not sure that "git add -u" would not accomplish the same
thing as these several commands.
-Peff
^ permalink raw reply
* Re: [PATCH v15 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
From: Pranit Bauva @ 2016-12-06 19:33 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <a4c7fec8-0e84-eb53-ca22-c369ce3facfa@gmx.net>
Hey Stephan,
On Thu, Nov 17, 2016 at 5:17 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index d84ba86..c542e8b 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -123,13 +123,40 @@ static int bisect_reset(const char *commit)
>> return bisect_clean_state();
>> }
>>
>> +static int is_expected_rev(const char *expected_hex)
>> +{
>> + struct strbuf actual_hex = STRBUF_INIT;
>> + int res = 0;
>> + if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {
>> + strbuf_trim(&actual_hex);
>> + res = !strcmp(actual_hex.buf, expected_hex);
>> + }
>> + strbuf_release(&actual_hex);
>> + return res;
>> +}
>
> I am not sure it does what it should.
>
> I would expect the following behavior from this function:
> - file does not exist (or is "broken") => return 0
> - actual_hex != expected_hex => return 0
> - otherwise return 1
>
> If I am not wrong, the code does the following instead:
> - file does not exist (or is "broken") => return 0
> - actual_hex != expected_hex => return 1
> - otherwise => return 0
Yeah, you are right. I should update this. Thanks for pointing it out.
>> +static int check_expected_revs(const char **revs, int rev_nr)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < rev_nr; i++) {
>> + if (!is_expected_rev(revs[i])) {
>> + unlink_or_warn(git_path_bisect_ancestors_ok());
>> + unlink_or_warn(git_path_bisect_expected_rev());
>> + return 0;
>> + }
>> + }
>> + return 0;
>> +}
>
> Here I am not sure what the function *should* do. However, I see that it
> basically mimics the behavior of the shell function (assuming
> is_expected_rev() is implemented correctly).
>
> I don't understand why the return value is int and not void. To avoid a
> "return 0;" line when calling this function?
Initially I thought I would be using the return value but now I
realize that it is meaningless to do so. Using void seems better. :)
>> @@ -167,6 +196,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> if (argc > 1)
>> die(_("--bisect-reset requires either zero or one arguments"));
>> return bisect_reset(argc ? argv[0] : NULL);
>> + case CHECK_EXPECTED_REVS:
>> + return check_expected_revs(argv, argc);
>
> I note that you check the correct number of arguments for some
> subcommands and you do not check it for some other subcommands like this
> one. (I don't care, I just want to mention it.)
Here we should be able to accept any number of arguments. I think it
would be good to add a non-zero check though just to maintain the
uniformity. Though this is something programmer needs to be careful
about rather than the user.
>> default:
>> die("BUG: unknown subcommand '%d'", cmdmode);
>> }
>
> ~Stephan
Regards,
Pranit Bauva
^ permalink raw reply
* Re: [PATCH v15 18/27] bisect--helper: `bisect_autostart` shell function in C
From: Pranit Bauva @ 2016-12-06 19:47 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <723476f6-2c8c-38df-1771-9a525196d9de@gmx.net>
Hey Stephan,
On Mon, Nov 21, 2016 at 1:45 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 502bf18..1767916 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -422,6 +425,7 @@ static int bisect_next(...)
>> {
>> int res, no_checkout;
>>
>> + bisect_autostart(terms);
>
> You are not checking for return values here. (The shell code simply
> exited if there is no tty, but you don't.)
True. I didn't notice it carefully. Thanks for pointing it out.
>> @@ -754,6 +758,32 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
>> return retval || bisect_auto_next(terms, NULL);
>> }
>>
>> +static int bisect_autostart(struct bisect_terms *terms)
>> +{
>> + if (is_empty_or_missing_file(git_path_bisect_start())) {
>> + const char *yesno;
>> + const char *argv[] = {NULL};
>> + fprintf(stderr, _("You need to start by \"git bisect "
>> + "start\"\n"));
>> +
>> + if (!isatty(0))
>
> isatty(STDIN_FILENO)?
Seems better.
>> + return 1;
>> +
>> + /*
>> + * TRANSLATORS: Make sure to include [Y] and [n] in your
>> + * translation. THe program will only accept English input
>
> Typo "THe"
Sure.
>> + * at this point.
>> + */
>
> Taking "at this point" into consideration, I think the Y and n can be
> easily translated now that it is in C. I guess, by using...
>
>> + yesno = git_prompt(_("Do you want me to do it for you "
>> + "[Y/n]? "), PROMPT_ECHO);
>> + if (starts_with(yesno, "n") || starts_with(yesno, "N"))
>
> ... starts_with(yesno, _("n")) || starts_with(yesno, _("N"))
> here (but not sure). However, this would be an extra patch on top of
> this series.
Can add it as an extra patch. Thanks for informing.
>> + exit(0);
>
> Shouldn't this also be "return 1;"? Saying "no" is the same outcome as
> not having a tty to ask for yes or no.
Yes.
>> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> {
>> enum {
>> @@ -790,6 +821,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> N_("find the next bisection commit"), BISECT_NEXT),
>> OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
>> N_("verify the next bisection state then find the next bisection state"), BISECT_AUTO_NEXT),
>> + OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
>> + N_("start the bisection if BISECT_START empty or missing"), BISECT_AUTOSTART),
>
> The word "is" is missing.
Sure. Thanks for going through these patches very carefully.
Regards,
Pranit Bauva
^ permalink raw reply
* Re: [PATCH] stash: disable renames when calling git-diff
From: Junio C Hamano @ 2016-12-06 19:51 UTC (permalink / raw)
To: Jeff King; +Cc: Matthew Patey, git
In-Reply-To: <20161206193154.vf7cd7lk5gyxrra5@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Dec 06, 2016 at 11:20:18AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> >> If you run:
>> >>
>> >> git -c diff.renames=false stash
>> >>
>> >> then it works.
>> >
>> > And here's a patch to fix it.
>>
>> Yuck. This obviously has easier to bite more people since we
>> enabled the renames by default. Thanks for a quick fix.
>>
>> I wonder why we are using "git diff" here, not the plumbing,
>> though.
>
> I don't think there's a plumbing command which works for diffing the
> working tree directly to a git tree. In the long run, it might be a good
> idea to remedy that.
I do not think that one is doing anything different from "git
diff-index --name-only -z HEAD".
> Though I'm not sure that "git add -u" would not accomplish the same
> thing as these several commands.
Yeah, it looks like "add -u" to me, too. Perhaps the script was old
enough that it didn't exist back then? I dunno.
^ permalink raw reply
* Re: [PATCH] stash: disable renames when calling git-diff
From: Jeff King @ 2016-12-06 20:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthew Patey, git
In-Reply-To: <xmqqd1h425vv.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 06, 2016 at 11:51:16AM -0800, Junio C Hamano wrote:
> > I don't think there's a plumbing command which works for diffing the
> > working tree directly to a git tree. In the long run, it might be a good
> > idea to remedy that.
>
> I do not think that one is doing anything different from "git
> diff-index --name-only -z HEAD".
> [...]
> Yeah, it looks like "add -u" to me, too. Perhaps the script was old
> enough that it didn't exist back then? I dunno.
Hmm, nope. It _was_ "git add -u" at one point and switched. See
7aa5d43cc (stash: Don't overwrite files that have gone from the index,
2010-04-18).
I think you are right that diff-index could work, though. I always
forget that without "--cached", diff-index looks at the working tree
files.
-Peff
^ permalink raw reply
* Re: [PATCH] stash: disable renames when calling git-diff
From: Junio C Hamano @ 2016-12-06 20:22 UTC (permalink / raw)
To: Jeff King; +Cc: Matthew Patey, git
In-Reply-To: <20161206201130.azfmqvtk3iettlx7@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I think you are right that diff-index could work, though. I always
> forget that without "--cached", diff-index looks at the working tree
> files.
I'll reword the log message while queuing. It was super surprising
to me to hear that there was something "git diff" did that three
"git diff-*" plumbing brothers couldn't do at the basic "what to
compare with what" level, as I wrote all four ;-)
^ permalink raw reply
* Re: [PATCH] stash: disable renames when calling git-diff
From: Jeff King @ 2016-12-06 20:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthew Patey, git
In-Reply-To: <xmqq37i024fy.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 06, 2016 at 12:22:25PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I think you are right that diff-index could work, though. I always
> > forget that without "--cached", diff-index looks at the working tree
> > files.
>
> I'll reword the log message while queuing. It was super surprising
> to me to hear that there was something "git diff" did that three
> "git diff-*" plumbing brothers couldn't do at the basic "what to
> compare with what" level, as I wrote all four ;-)
Oops, our emails just crossed; I just sent a revised patch.
I know there are a few cases that git-diff can handle that the others
can't, but I think they are all direct blob-to-blob comparisons.
-Peff
^ permalink raw reply
* Re: [PATCH] stash: disable renames when calling git-diff
From: Junio C Hamano @ 2016-12-06 20:31 UTC (permalink / raw)
To: Jeff King; +Cc: Matthew Patey, git
In-Reply-To: <xmqq37i024fy.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> I think you are right that diff-index could work, though. I always
>> forget that without "--cached", diff-index looks at the working tree
>> files.
>
> I'll reword the log message while queuing.
The last paragraph became like so:
The correct solution is to move to an appropriate plumbing
command. But in the short term lets ask git-diff not to respect
renames.
^ permalink raw reply
* Re: [PATCH] stash: disable renames when calling git-diff
From: Jeff King @ 2016-12-06 20:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Charles Bailey, Matthew Patey, git
In-Reply-To: <20161206201130.azfmqvtk3iettlx7@sigill.intra.peff.net>
On Tue, Dec 06, 2016 at 03:11:30PM -0500, Jeff King wrote:
> > Yeah, it looks like "add -u" to me, too. Perhaps the script was old
> > enough that it didn't exist back then? I dunno.
>
> Hmm, nope. It _was_ "git add -u" at one point and switched. See
> 7aa5d43cc (stash: Don't overwrite files that have gone from the index,
> 2010-04-18).
>
> I think you are right that diff-index could work, though. I always
> forget that without "--cached", diff-index looks at the working tree
> files.
Here it is in patch form. Perhaps I am missing some subtle case that
diff-index would not handle, but it does pass the test suite. And
looking over the original thread, I don't see any particular reason to
prefer git-diff.
+cc Charles just in case he remembers anything.
-- >8 --
Subject: [PATCH] stash: prefer plumbing over git-diff
When creating a stash, we need to look at the diff between
the working tree and HEAD, and do so using the git-diff
porcelain. Because git-diff enables porcelain config like
renames by default, this causes at least one problem. The
--name-only format will not mention the source side of a
rename, meaning we will fail to stash a deletion that is
part of a rename.
We could fix that case by passing --no-renames, but this is
a symptom of a larger problem. We should be using the
diff-index plumbing here, which does not have renames
enabled by default, and also does not respect any
potentially confusing config options.
Reported-by: Matthew Patey <matthew.patey2167@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
git-stash.sh | 2 +-
t/t3903-stash.sh | 9 +++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/git-stash.sh b/git-stash.sh
index 4546abaae..10c284d1a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -115,7 +115,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
- git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
+ git diff-index --name-only -z HEAD -- >"$TMP-stagenames" &&
git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
git write-tree &&
rm -f "$TMPindex"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index e1a6ccc00..2de3e18ce 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -766,4 +766,13 @@ test_expect_success 'stash list --cc shows combined diff' '
test_cmp expect actual
'
+test_expect_success 'stash is not confused by partial renames' '
+ mv file renamed &&
+ git add renamed &&
+ git stash &&
+ git stash apply &&
+ test_path_is_file renamed &&
+ test_path_is_missing file
+'
+
test_done
--
2.11.0.191.gdb26c57
^ permalink raw reply related
* Re: [PATCH] stash: disable renames when calling git-diff
From: Junio C Hamano @ 2016-12-06 20:36 UTC (permalink / raw)
To: Jeff King; +Cc: Charles Bailey, Matthew Patey, git
In-Reply-To: <20161206202521.zxgxqsetgqejsyl3@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Here it is in patch form. Perhaps I am missing some subtle case that
> diff-index would not handle, but it does pass the test suite. And
> looking over the original thread, I don't see any particular reason to
> prefer git-diff.
Ah, our mails crossed ;-) I am reasonably sure that "diff HEAD" and
"diff-index HEAD" would behave identically.
> @@ -115,7 +115,7 @@ create_stash () {
> git read-tree --index-output="$TMPindex" -m $i_tree &&
> GIT_INDEX_FILE="$TMPindex" &&
> export GIT_INDEX_FILE &&
> - git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
> + git diff-index --name-only -z HEAD -- >"$TMP-stagenames" &&
> git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
> git write-tree &&
> rm -f "$TMPindex"
Will queue this one instead. Thanks.
^ permalink raw reply
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Ramsay Jones @ 2016-12-06 20:43 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: GIT Mailing-list
In-Reply-To: <20161206182648.sxoftkd4hjhvenaf@sigill.intra.peff.net>
On 06/12/16 18:26, Jeff King wrote:
> On Tue, Dec 06, 2016 at 10:17:55AM -0800, Junio C Hamano wrote:
>
>> Yup, that is what I meant to say with "that is already possible" and
>> we are on the same page. As all three of us seem to be happy with
>> just dropping --abbrev and letting describe do its default thing (as
>> configured by whoever is doing the build), let's do so.
>>
>> -- >8 --
>> From: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> Date: Sun, 4 Dec 2016 20:45:59 +0000
>> Subject: [PATCH] GIT-VERSION-GEN: do not force abbreviation length used by 'describe'
>
> Thanks, this is a nice summary of the discussion, and the patch is
> obviously correct.
Yep, looks very good to me! (I had started to write a commit
message for this patch, when your email arrived. Needless to
say, but your message is much better than mine!)
Thanks!
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH v2] tag, branch, for-each-ref: add --ignore-case for sorting and filtering
From: Junio C Hamano @ 2016-12-06 21:11 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, karthik.188
In-Reply-To: <20161204025225.11158-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This options makes sorting ignore case, which is great when you have
> branches named bug-12-do-something, Bug-12-do-some-more and
> BUG-12-do-what and want to group them together. Sorting externally may
> not be an option because we lose coloring and column layout from
> git-branch and git-tag.
>
> The same could be said for filtering, but it's probably less important
> because you can always go with the ugly pattern [bB][uU][gG]-* if you're
> desperate.
>
> You can't have case-sensitive filtering and case-insensitive sorting (or
> the other way around) with this though. For branch and tag, that should
> be no problem. for-each-ref, as a plumbing, might want finer control.
> But we can always add --{filter,sort}-ignore-case when there is a need
> for it.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
It took me a while to figure out the interactions with topics in
flight, but I think I resolved it correctly now. There was a topic
that added "--format" to branch and tag.
Will be pushed out as part of today's integration cycle.
Thanks.
^ permalink raw reply
* Re: "git add -p ." raises an unexpected "warning: empty strings as pathspecs will be made invalid in upcoming releases. please use . instead if you meant to match all paths"
From: Junio C Hamano @ 2016-12-06 21:21 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, Emily Xie, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161206180400.GA103573@google.com>
Brandon Williams <bmwill@google.com> writes:
> On 11/30, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> forgot to Cc: the author of the
>> most relevant change to the issue, d426430e6e ("pathspec: warn on
>> empty strings as pathspec", 2016-06-22).
>> ...
>
> I've been doing a bit of work trying to clean up the pathspec
> initialization code and I believe this can be fixed without
> having to add in this work around. The code which does the munging is
> always trying to prefix the pathspec regardless if there is a prefix or
> not. If instead its changed to only try and prefix the original if
> there is indeed a prefix, then it should fix the munging.
Thanks; sounds like a plan.
^ permalink raw reply
* Re: [PATCH] xdiff: Do not enable XDL_FAST_HASH by default
From: Junio C Hamano @ 2016-12-06 21:23 UTC (permalink / raw)
To: Jeff King; +Cc: Anders Kaseorg, git, Thomas Rast
In-Reply-To: <20161201045243.mlr7wqvkbm2yd37m@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This is a nice incremental step in the sense that people can still
> enable it if they want to in order to time it, play with it, etc. But
> given what we know, I wonder if the help text here should warn people.
>
> Or I guess we could move straight to dropping it entirely.
>
> Here's what that patch might look like (I retimed it just be sure, and
> was sad to see that it really _is_ making some cases faster. But I still
> think slower-but-predictable is a better default).
I like this version that drops quite a lot of code ;-)
> Subject: [PATCH] xdiff: drop XDL_FAST_HASH
> ...
> The idea of XDL_FAST_HASH is to speed up the hash
> computation. But the generated hashes have worse collision
> behavior. This means that in some cases it speeds diffs up
> (running "git log -p" on git.git improves by ~8% with it),
> but in others it can slow things down. One pathological case
> saw over a 100x slowdown[1].
>
> There may be a better hash function that covers both
> properties, but in the meantime we are better off with the
> original hash. It's slightly slower in the common case, but
> it has fewer surprising pathological cases.
>
> [1] http://public-inbox.org/git/20141222041944.GA441@peff.net/
^ permalink raw reply
* Re: [PATCH v15 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
From: Pranit Bauva @ 2016-12-06 21:14 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <b54f7f46-a3c0-3334-24fa-e8d1e7d8f653@gmx.net>
Hey Stephan,
On Fri, Nov 18, 2016 at 3:02 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 317d671..6a5878c 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
> [...]
>> +static int bisect_terms(struct bisect_terms *terms, const char **argv, int argc)
>> +{
>> + int i;
>> + const char bisect_term_usage[] =
>> +"git bisect--helper --bisect-terms [--term-good | --term-bad | ]"
>> +"--term-old | --term-new";
>
> Three things:
>
> (1) Is that indentation intentional?
Yes it was intentional but now I cannot recollect why. I think it was
because I found something similar. Nevertheless, I will fix this
indentation/
> (2) You have a "]" at the end of the first part of the string instead of
> the end of the second part.
This should be corrected.
> (3) After the correction, bisect_term_usage and
> git_bisect_helper_usage[7] are the same strings. I don't recommend to
> use git_bisect_helper_usage[7] instead because keeping the index
> up-to-date is a maintenance hell. (At the end of your patch series it is
> a 3 instead of a 7.) However, if - for whatever reason - the usage of
> bisect--helper --bisect-terms changes, you always have to sync the two
> strings which is also nasty....
>
>> +
>> + if (get_terms(terms))
>> + return error(_("no terms defined"));
>> +
>> + if (argc > 1) {
>> + usage(bisect_term_usage);
>> + return -1;
>> + }
>
> ...and since you only use it once, why not simply do something like
>
> return error(_("--bisect-term requires exactly one argument"));
>
> and drop the definition of bisect_term_usage.
Sure that would be better.
>> +
>> + if (argc == 0) {
>> + printf(_("Your current terms are %s for the old state\nand "
>> + "%s for the new state.\n"), terms->term_good,
>> + terms->term_bad);
>
> Very minor: It improves the readability if you'd split the string after
> the \n and put the "and "in the next line.
Ah. This is because of the message. If I do the other way, then it
won't match the output in one of the tests in t/t6030 thus, I am
keeping it that way in order to avoid modifying the file t/t6030.
>> + return 0;
>> + }
>> +
>> + for (i = 0; i < argc; i++) {
>> + if (!strcmp(argv[i], "--term-good"))
>> + printf("%s\n", terms->term_good);
>> + else if (!strcmp(argv[i], "--term-bad"))
>> + printf("%s\n", terms->term_bad);
>> + else
>> + die(_("invalid argument %s for 'git bisect "
>> + "terms'.\nSupported options are: "
>> + "--term-good|--term-old and "
>> + "--term-bad|--term-new."), argv[i]);
>
> Hm, "return error(...)" and "die(...)" seems to be quasi-equivalent in
> this case. Because I am always looking from a library perspective, I'd
> prefer "return error(...)".
I should use return error()
>> @@ -429,6 +492,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> terms.term_bad = xstrdup(argv[1]);
>> res = bisect_next_check(&terms, argc == 3 ? argv[2] : NULL);
>> break;
>> + case BISECT_TERMS:
>> + if (argc > 1)
>> + die(_("--bisect-terms requires 0 or 1 argument"));
>> + res = bisect_terms(&terms, argv, argc);
>> + break;
>
> Also here: "terms" is leaking...
Will have to free it.
> ~Stephan
^ permalink raw reply
* Re: [PATCH v15 09/27] bisect--helper: `bisect_write` shell function in C
From: Pranit Bauva @ 2016-12-06 21:32 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <ceb2c50b-0ca7-115d-eb0e-316389569e36@gmx.net>
Hey Stephan,
On Thu, Nov 17, 2016 at 3:10 PM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi,
>
> I've only got some minors to mention here ;)
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index c542e8b..3f19b68 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -19,9 +19,15 @@ static const char * const git_bisect_helper_usage[] = {
>> N_("git bisect--helper --write-terms <bad_term> <good_term>"),
>> N_("git bisect--helper --bisect-clean-state"),
>> N_("git bisect--helper --bisect-reset [<commit>]"),
>> + N_("git bisect--helper --bisect-write <state> <revision> <TERM_GOOD> <TERM_BAD> [<nolog>]"),
>> NULL
>> };
>
> I wouldn't write "<TERM_GOOD <TERM_BAD>" in capital letters. I'd use
> something like "<good_term> <bad_term>" as you have used for
> --write-terms. Note that "git bisect --help" uses "<term-old>
> <term-new>" in that context.
Keeping it in small does give less strain to the eye ;)
>> @@ -149,6 +155,63 @@ static int check_expected_revs(const char **revs, int rev_nr)
>> return 0;
>> }
>>
>> +static int bisect_write(const char *state, const char *rev,
>> + const struct bisect_terms *terms, int nolog)
>> +{
>> + struct strbuf tag = STRBUF_INIT;
>> + struct strbuf commit_name = STRBUF_INIT;
>> + struct object_id oid;
>> + struct commit *commit;
>> + struct pretty_print_context pp = {0};
>> + FILE *fp = NULL;
>> + int retval = 0;
>> +
>> + if (!strcmp(state, terms->term_bad))
>> + strbuf_addf(&tag, "refs/bisect/%s", state);
>> + else if (one_of(state, terms->term_good, "skip", NULL))
>> + strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
>> + else {
>> + error(_("Bad bisect_write argument: %s"), state);
>> + retval = -1;
>> + goto finish;
>> + }
>> +
>> + if (get_oid(rev, &oid)) {
>> + error(_("couldn't get the oid of the rev '%s'"), rev);
>> + retval = -1;
>> + goto finish;
>> + }
>> +
>> + if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
>> + UPDATE_REFS_MSG_ON_ERR)) {
>> + retval = -1;
>> + goto finish;
>> + }
>
> I'd like to mention that the "goto fail;" trick could apply in this
> function, too.
Sure!
>> @@ -156,9 +219,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> WRITE_TERMS,
>> BISECT_CLEAN_STATE,
>> BISECT_RESET,
>> - CHECK_EXPECTED_REVS
>> + CHECK_EXPECTED_REVS,
>> + BISECT_WRITE
>> } cmdmode = 0;
>> - int no_checkout = 0;
>> + int no_checkout = 0, res = 0;
>
> Why do you do this "direct return" -> "set res and return res" transition?
> You don't need it in this patch, you do not need it in the subsequent
> patches (you always set "res" exactly once after the initialization),
> and you don't need cleanup code in this function.
Initially I was using strbuf but then I switched to const char *
according to Junio's suggestion. It seems that in this version I have
forgot to free the terms.
>> struct option options[] = {
>> OPT_CMDMODE(0, "next-all", &cmdmode,
>> N_("perform 'git bisect next'"), NEXT_ALL),
>> @@ -170,10 +234,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> N_("reset the bisection state"), BISECT_RESET),
>> OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
>> N_("check for expected revs"), CHECK_EXPECTED_REVS),
>> + OPT_CMDMODE(0, "bisect-write", &cmdmode,
>> + N_("write out the bisection state in BISECT_LOG"), BISECT_WRITE),
>
> That info text is confusing, especially considering that there is a
> "nolog" option. I think the action of bisect-write is two-fold: (1)
> update the refs, (2) log.
I agree that it is confusing. I couldn't find a better way to describe
it and since this would be gone after the whole conversion, I didn't
bother putting more efforts there.
Regards,
Pranit Bauva
^ permalink raw reply
* [PATCH 01/17] mv: convert to using pathspec struct interface
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Convert the 'internal_copy_pathspec()' function to use the pathspec
struct interface from using the deprecated 'get_pathspec()' interface.
In addition to this, fix a memory leak caused by only duplicating some
of the pathspec elements. Instead always duplicate all of the the
pathspec elements as an intermediate step (with modificationed based on
the passed in flags). This way the intermediate strings can then be
freed prior to duplicating the result of parse_pathspec (which contains
each of the elements with the prefix prepended).
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/mv.c | 45 ++++++++++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 13 deletions(-)
diff --git a/builtin/mv.c b/builtin/mv.c
index 2f43877..4df4a12 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -4,6 +4,7 @@
* Copyright (C) 2006 Johannes Schindelin
*/
#include "builtin.h"
+#include "pathspec.h"
#include "lockfile.h"
#include "dir.h"
#include "cache-tree.h"
@@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char *prefix,
{
int i;
const char **result;
+ struct pathspec ps;
ALLOC_ARRAY(result, count + 1);
- COPY_ARRAY(result, pathspec, count);
- result[count] = NULL;
+
+ /* Create an intermediate copy of the pathspec based on the flags */
for (i = 0; i < count; i++) {
- int length = strlen(result[i]);
+ int length = strlen(pathspec[i]);
int to_copy = length;
+ char *it;
while (!(flags & KEEP_TRAILING_SLASH) &&
- to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
+ to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
to_copy--;
- if (to_copy != length || flags & DUP_BASENAME) {
- char *it = xmemdupz(result[i], to_copy);
- if (flags & DUP_BASENAME) {
- result[i] = xstrdup(basename(it));
- free(it);
- } else
- result[i] = it;
- }
+
+ it = xmemdupz(pathspec[i], to_copy);
+ if (flags & DUP_BASENAME) {
+ result[i] = xstrdup(basename(it));
+ free(it);
+ } else
+ result[i] = it;
+ }
+ result[count] = NULL;
+
+ parse_pathspec(&ps,
+ PATHSPEC_ALL_MAGIC &
+ ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
+ PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,
+ prefix, result);
+ assert(count == ps.nr);
+
+ /* Copy the pathspec and free the old intermediate strings */
+ for (i = 0; i < count; i++) {
+ const char *match = xstrdup(ps.items[i].match);
+ free((char *) result[i]);
+ result[i] = match;
}
- return get_pathspec(prefix, result);
+
+ clear_pathspec(&ps);
+ return result;
}
static const char *add_slash(const char *path)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 00/17] pathspec cleanup
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
The intent of this series is to cleanup some of the pathspec initialization
code as well as finally migrating the remaining users of the _raw field or
get_pathspec() to the pathspec struct interface. This way both the _raw field
and get_pathspec() can be removed from the codebase. This also removes the
functionality where parse_pathspec() modified the const char * argv array that
was passed in (which felt kind of odd to me as I wouldn't have expected the
passed in array to be modified).
I also noticed that there are memory leaks associated with the 'original' and
'match' strings. To fix this the pathspec struct needed to take ownership of
the memory for these fields so that they can be cleaned up when clearing the
pathspec struct.
Most of the work went to simplifying the prefix_pathspec function. This
consisted of factoring out long sections of code into their own helper
functions. The overall result is a much more readable function.
Brandon Williams (17):
mv: convert to using pathspec struct interface
dir: convert create_simplify to use the pathspec struct interface
dir: convert fill_directory to use the pathspec struct interface
ls-tree: convert show_recursive to use the pathspec struct interface
pathspec: remove the deprecated get_pathspec function
pathspec: copy and free owned memory
mv: small code cleanup
pathspec: remove unused variable from unsupported_magic
pathspec: always show mnemonic and name in unsupported_magic
pathspec: simpler logic to prefix original pathspec elements
pathspec: factor global magic into its own function
pathspec: create parse_short_magic function
pathspec: create parse_long_magic function
pathspec: create parse_element_magic helper
pathspec: create strip submodule slash helpers
pathspec: small readability changes
pathspec: remove outdated comment
Documentation/technical/api-setup.txt | 2 -
builtin/ls-tree.c | 12 +-
builtin/mv.c | 44 +++-
cache.h | 1 -
dir.c | 28 +--
pathspec.c | 449 +++++++++++++++++++---------------
pathspec.h | 5 +-
7 files changed, 301 insertions(+), 240 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Convert 'create_simplify()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
dir.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/dir.c b/dir.c
index bfa8c8a..7df292b 100644
--- a/dir.c
+++ b/dir.c
@@ -1787,25 +1787,24 @@ static int cmp_name(const void *p1, const void *p2)
return name_compare(e1->name, e1->len, e2->name, e2->len);
}
-static struct path_simplify *create_simplify(const char **pathspec)
+static struct path_simplify *create_simplify(const struct pathspec *pathspec)
{
- int nr, alloc = 0;
+ int i;
struct path_simplify *simplify = NULL;
- if (!pathspec)
+ if (!pathspec || !pathspec->nr)
return NULL;
- for (nr = 0 ; ; nr++) {
+ ALLOC_ARRAY(simplify, pathspec->nr + 1);
+ for (i = 0; i < pathspec->nr; i++) {
const char *match;
- ALLOC_GROW(simplify, nr + 1, alloc);
- match = *pathspec++;
- if (!match)
- break;
- simplify[nr].path = match;
- simplify[nr].len = simple_length(match);
+ match = pathspec->items[i].match;
+ simplify[i].path = match;
+ simplify[i].len = pathspec->items[i].nowildcard_len;
}
- simplify[nr].path = NULL;
- simplify[nr].len = 0;
+ simplify[i].path = NULL;
+ simplify[i].len = 0;
+
return simplify;
}
@@ -2036,7 +2035,7 @@ int read_directory(struct dir_struct *dir, const char *path, int len, const stru
* subset of positive ones, which has no impacts on
* create_simplify().
*/
- simplify = create_simplify(pathspec ? pathspec->_raw : NULL);
+ simplify = create_simplify(pathspec);
untracked = validate_untracked_cache(dir, len, pathspec);
if (!untracked)
/*
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 03/17] dir: convert fill_directory to use the pathspec struct interface
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Convert 'fill_directory()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
dir.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/dir.c b/dir.c
index 7df292b..8730a4f 100644
--- a/dir.c
+++ b/dir.c
@@ -188,7 +188,8 @@ int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)
len = common_prefix_len(pathspec);
/* Read the directory and prune it */
- read_directory(dir, pathspec->nr ? pathspec->_raw[0] : "", len, pathspec);
+ read_directory(dir, pathspec->nr ? pathspec->items[0].match : "",
+ len, pathspec);
return len;
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 10/17] pathspec: simpler logic to prefix original pathspec elements
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
The logic used to prefix an original pathspec element with 'prefix'
magic is more general purpose and can be used for more than just short
magic. Remove the extra code paths and rename 'prefix_short_magic' to
'prefix_magic' to better indicate that it can be used in more general
situations.
Also, slightly change the logic which decides when to prefix the
original element in order to prevent a pathspec of "." from getting
converted to "" (empty string).
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 33 +++++++++++++--------------------
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 159f6db..5afebd3 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -74,13 +74,12 @@ static struct pathspec_magic {
{ PATHSPEC_EXCLUDE, '!', "exclude" },
};
-static void prefix_short_magic(struct strbuf *sb, int prefixlen,
- unsigned short_magic)
+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
{
int i;
strbuf_addstr(sb, ":(");
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
- if (short_magic & pathspec_magic[i].bit) {
+ if (magic & pathspec_magic[i].bit) {
if (sb->buf[sb->len - 1] != '(')
strbuf_addch(sb, ',');
strbuf_addstr(sb, pathspec_magic[i].name);
@@ -110,8 +109,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
static int glob_global = -1;
static int noglob_global = -1;
static int icase_global = -1;
- unsigned magic = 0, short_magic = 0, global_magic = 0;
- const char *copyfrom = elt, *long_magic_end = NULL;
+ unsigned magic = 0, element_magic = 0, global_magic = 0;
+ const char *copyfrom = elt;
char *match;
int i, pathspec_prefix = -1;
@@ -165,7 +164,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
if (strlen(pathspec_magic[i].name) == len &&
!strncmp(pathspec_magic[i].name, copyfrom, len)) {
- magic |= pathspec_magic[i].bit;
+ element_magic |= pathspec_magic[i].bit;
break;
}
if (starts_with(copyfrom, "prefix:")) {
@@ -184,7 +183,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
if (*copyfrom != ')')
die(_("Missing ')' at the end of pathspec magic in '%s'"), elt);
- long_magic_end = copyfrom;
copyfrom++;
} else {
/* shorthand */
@@ -197,7 +195,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
break;
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
if (pathspec_magic[i].mnemonic == ch) {
- short_magic |= pathspec_magic[i].bit;
+ element_magic |= pathspec_magic[i].bit;
break;
}
if (ARRAY_SIZE(pathspec_magic) <= i)
@@ -208,7 +206,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
copyfrom++;
}
- magic |= short_magic;
+ magic |= element_magic;
/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
if (noglob_global && !(magic & PATHSPEC_GLOB))
@@ -243,18 +241,13 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
* Prefix the pathspec (keep all magic) and assign to
* original. Useful for passing to another command.
*/
- if (flags & PATHSPEC_PREFIX_ORIGIN) {
+ if ((flags & PATHSPEC_PREFIX_ORIGIN) &&
+ prefixlen && !literal_global) {
struct strbuf sb = STRBUF_INIT;
- if (prefixlen && !literal_global) {
- /* Preserve the actual prefix length of each pattern */
- if (short_magic)
- prefix_short_magic(&sb, prefixlen, short_magic);
- else if (long_magic_end) {
- strbuf_add(&sb, elt, long_magic_end - elt);
- strbuf_addf(&sb, ",prefix:%d)", prefixlen);
- } else
- strbuf_addf(&sb, ":(prefix:%d)", prefixlen);
- }
+
+ /* Preserve the actual prefix length of each pattern */
+ prefix_magic(&sb, prefixlen, element_magic);
+
strbuf_addstr(&sb, match);
item->original = strbuf_detach(&sb, NULL);
} else {
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 08/17] pathspec: remove unused variable from unsupported_magic
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Removed unused variable 'n' from the 'unsupported_magic()' function.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 8f367f0..ec0d590 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -333,8 +333,8 @@ static void NORETURN unsupported_magic(const char *pattern,
unsigned short_magic)
{
struct strbuf sb = STRBUF_INIT;
- int i, n;
- for (n = i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+ int i;
+ for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
const struct pathspec_magic *m = pathspec_magic + i;
if (!(magic & m->bit))
continue;
@@ -344,7 +344,6 @@ static void NORETURN unsupported_magic(const char *pattern,
strbuf_addf(&sb, "'%c'", m->mnemonic);
else
strbuf_addf(&sb, "'%s'", m->name);
- n++;
}
/*
* We may want to substitute "this command" with a command
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 09/17] pathspec: always show mnemonic and name in unsupported_magic
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
For better clarity, always show the mnemonic and name of the unsupported
magic being used. This lets users have a more clear understanding of
what magic feature isn't supported. And if they supplied a mnemonic,
the user will be told what its corresponding name is which will allow
them to more easily search the man pages for that magic type.
This also avoids passing an extra parameter around the pathspec
initialization code.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index ec0d590..159f6db 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -68,7 +68,7 @@ static struct pathspec_magic {
const char *name;
} pathspec_magic[] = {
{ PATHSPEC_FROMTOP, '/', "top" },
- { PATHSPEC_LITERAL, 0, "literal" },
+ { PATHSPEC_LITERAL,'\0', "literal" },
{ PATHSPEC_GLOB, '\0', "glob" },
{ PATHSPEC_ICASE, '\0', "icase" },
{ PATHSPEC_EXCLUDE, '!', "exclude" },
@@ -102,7 +102,6 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,
* string cannot express such a case.
*/
static unsigned prefix_pathspec(struct pathspec_item *item,
- unsigned *p_short_magic,
unsigned flags,
const char *prefix, int prefixlen,
const char *elt)
@@ -210,7 +209,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
magic |= short_magic;
- *p_short_magic = short_magic;
/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
if (noglob_global && !(magic & PATHSPEC_GLOB))
@@ -329,8 +327,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
}
static void NORETURN unsupported_magic(const char *pattern,
- unsigned magic,
- unsigned short_magic)
+ unsigned magic)
{
struct strbuf sb = STRBUF_INIT;
int i;
@@ -340,8 +337,9 @@ static void NORETURN unsupported_magic(const char *pattern,
continue;
if (sb.len)
strbuf_addch(&sb, ' ');
- if (short_magic & m->bit)
- strbuf_addf(&sb, "'%c'", m->mnemonic);
+
+ if (m->mnemonic)
+ strbuf_addf(&sb, "'(%c)%s'", m->mnemonic, m->name);
else
strbuf_addf(&sb, "'%s'", m->name);
}
@@ -413,10 +411,9 @@ void parse_pathspec(struct pathspec *pathspec,
prefixlen = prefix ? strlen(prefix) : 0;
for (i = 0; i < n; i++) {
- unsigned short_magic;
entry = argv[i];
- item[i].magic = prefix_pathspec(item + i, &short_magic,
+ item[i].magic = prefix_pathspec(item + i,
flags,
prefix, prefixlen, entry);
if ((flags & PATHSPEC_LITERAL_PATH) &&
@@ -426,8 +423,7 @@ void parse_pathspec(struct pathspec *pathspec,
nr_exclude++;
if (item[i].magic & magic_mask)
unsupported_magic(entry,
- item[i].magic & magic_mask,
- short_magic);
+ item[i].magic & magic_mask);
if ((flags & PATHSPEC_SYMLINK_LEADING_PATH) &&
has_symlink_leading_path(item[i].match, item[i].len)) {
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 16/17] pathspec: small readability changes
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
A few small changes to improve readability. This is done by grouping related
assignments, adding blank lines, ensuring lines are <80 characters, etc.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 41aa213..8a07b02 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
if ((magic & PATHSPEC_LITERAL) && (magic & PATHSPEC_GLOB))
die(_("%s: 'literal' and 'glob' are incompatible"), elt);
+ /* Create match string which will be used for pathspec matching */
if (pathspec_prefix >= 0) {
match = xstrdup(copyfrom);
prefixlen = pathspec_prefix;
@@ -341,11 +342,16 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
match = xstrdup(copyfrom);
prefixlen = 0;
} else {
- match = prefix_path_gently(prefix, prefixlen, &prefixlen, copyfrom);
+ match = prefix_path_gently(prefix, prefixlen,
+ &prefixlen, copyfrom);
if (!match)
die(_("%s: '%s' is outside repository"), elt, copyfrom);
}
+
item->match = match;
+ item->len = strlen(item->match);
+ item->prefix = prefixlen;
+
/*
* Prefix the pathspec (keep all magic) and assign to
* original. Useful for passing to another command.
@@ -362,8 +368,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
} else {
item->original = xstrdup(elt);
}
- item->len = strlen(item->match);
- item->prefix = prefixlen;
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
strip_submodule_slash_cheap(item);
@@ -371,13 +375,14 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
strip_submodule_slash_expensive(item);
- if (magic & PATHSPEC_LITERAL)
+ if (magic & PATHSPEC_LITERAL) {
item->nowildcard_len = item->len;
- else {
+ } else {
item->nowildcard_len = simple_length(item->match);
if (item->nowildcard_len < prefixlen)
item->nowildcard_len = prefixlen;
}
+
item->flags = 0;
if (magic & PATHSPEC_GLOB) {
/*
--
2.8.0.rc3.226.g39d4020
^ 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