* Re: [PATCH] t3008: find test-tool through path lookup
From: Johannes Schindelin @ 2020-01-01 22:19 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
In-Reply-To: <2dc0b0f3-19ce-62ab-5af9-fdb2d05b00bd@kdbg.org>
Hi Hannes,
On Sun, 22 Dec 2019, Johannes Sixt wrote:
> Do not use $GIT_BUILD_DIR without quotes; it may contain spaces and be
> split into fields. But it is not necessary to access test-tool with an
> absolute path in the first place as it can be found via path lookup.
> Remove the explicit path.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> Found this today coincidentally when I happened to look at the terminal
> with test output at the right time and saw a suspicous error message
> that included just one half of the build directory path.
This patch looks good to me.
Thanks,
Dscho
>
> -- Hannes
>
> t/t3008-ls-files-lazy-init-name-hash.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t3008-ls-files-lazy-init-name-hash.sh b/t/t3008-ls-files-lazy-init-name-hash.sh
> index 64f047332b..85f3704958 100755
> --- a/t/t3008-ls-files-lazy-init-name-hash.sh
> +++ b/t/t3008-ls-files-lazy-init-name-hash.sh
> @@ -4,7 +4,7 @@ test_description='Test the lazy init name hash with various folder structures'
>
> . ./test-lib.sh
>
> -if test 1 -eq $($GIT_BUILD_DIR/t/helper/test-tool online-cpus)
> +if test 1 -eq $(test-tool online-cpus)
> then
> skip_all='skipping lazy-init tests, single cpu'
> test_done
> --
> 2.24.1.524.gae569673ff
>
>
^ permalink raw reply
* Re: [PATCH v2 9/9] ci: include the built-in `git add -i` in the `linux-gcc` job
From: Johannes Schindelin @ 2020-01-01 22:10 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano
In-Reply-To: <600bb1f7-b54a-3bf7-40ae-af656768a752@gmail.com>
Hi Stolee,
On Thu, 26 Dec 2019, Derrick Stolee wrote:
> On 12/25/2019 6:57 AM, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > This job runs the test suite twice, once in regular mode, and once with
> > a whole slew of `GIT_TEST_*` variables set.
> >
> > Now that the built-in version of `git add --interactive` is
> > feature-complete, let's also throw `GIT_TEST_ADD_I_USE_BUILTIN` into
> > that fray.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > ci/run-build-and-tests.sh | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> > index ff0ef7f08e..4df54c4efe 100755
> > --- a/ci/run-build-and-tests.sh
> > +++ b/ci/run-build-and-tests.sh
> > @@ -20,6 +20,7 @@ linux-gcc)
> > export GIT_TEST_OE_DELTA_SIZE=5
> > export GIT_TEST_COMMIT_GRAPH=1
> > export GIT_TEST_MULTI_PACK_INDEX=1
> > + export GIT_TEST_ADD_I_USE_BUILTIN=1
> > make test
>
> I see that I need to add this to the test-coverage builds.
Thank you for catching this!
It makes me wonder whether the test-coverage builds should use some `sed`
invocation on the `ci/run-build-and-tests.sh` script, though, so that you
do not have to edit the Azure Pipelines definition manually all the time?
Ciao,
Dscho
^ permalink raw reply
* Re: Propose promoting 'contrib/rerere-train.sh' to command
From: Johannes Schindelin @ 2020-01-01 22:06 UTC (permalink / raw)
To: Tom Miller; +Cc: Jeff King, Junio C Hamano, git
In-Reply-To: <BZBZ3Y9WU7J7.3PGDZKEYVQ165@penguin>
Hi,
On Sun, 22 Dec 2019, Tom Miller wrote:
> On Sun Dec 22, 2019 at 2:58 AM, Jeff King wrote:
> > On Sat, Dec 21, 2019 at 03:52:53PM -0800, Junio C Hamano wrote:
> >
> >
> > > Jeff King <peff@peff.net> writes:
> > >
> > > > The situations where I need rerere-train don't come up often, but when
> > > > they do, it has always worked easily and without hiccups for me. So
> > > > perhaps there are lurking gotchas that Junio might know about, but AFAIK
> > > > the quality is high enough for it to be part of normal Git.
> > >
> > > I actually suspect that rewriting has a high chance of initially
> > > degrading the quality, so we should take a two step approach if we
> > > really want it as part of the core distribution. As to the UI, I
> > > think "git rerere train a..b" would be a good one, but if the
> > > scripted version is of high quality (I haven't looked at it for a
> > > long time---even though I used it for a couple of times a year in
> > > recent years), perhaps we can add it as "git-rerere--train"
> > > subcommand that is spawned from "builtin/rerere.c" for the first
> > > cut?
> >
> >
> > Yeah, I'd be pretty happy with that, too.
> >
> >
> > I just suspect its ultimate fate is conversion to C, given the general
> > trend. And converting it to C that just calls out to other git commands
> > via run_command would presumably behave just like the original, leaving
> > the more challenging and error-prone conversion for later. Hopefully any
> > upgrade to "real Git command" would include some tests, though. :)
> >
> >
> > -Peff
>
> Thanks for the feedback everyone! This is roughly the feedback I
> expected. I think moving the shell version first will give more time to
> focus on getting a strong test harness in place and a some decent
> documentation. I happy with doing this in a multi phase approach as it
> introduces less risk. When I get to the point of writing it in C I will
> try to do the work internally by looking at commits as Jeff has
> suggested, and fall back to run commands if I have to.
As the person who traditionally ends up being stuck with such conversions,
I would like to point out that the rerere-train script has not changed _in
years_ and that there is little point in trying to let it "cook" some more
in the scripted form, _in particular_ because this would prevent the
command from having access to the more powerful C API for future
improvements.
One very obvious enhancement would be to perform the training in memory,
without even clobbering the worktree. That's simply not a thing that a
shell script can perform.
Food for thought,
Dscho
^ permalink raw reply
* Re: [PATCH 2/2] rebase: fix saving of --signoff state for am-based rebases
From: Johannes Schindelin @ 2020-01-01 22:01 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren via GitGitGadget, git, plroskin, Elijah Newren
In-Reply-To: <xmqqsgle94o3.fsf@gitster-ct.c.googlers.com>
Hi,
On Fri, 20 Dec 2019, Junio C Hamano wrote:
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > This was an error introduced in the conversion from shell in commit
> > 21853626eac5 ("built-in rebase: call `git am` directly", 2019-01-18),
> > which was noticed by a random browsing of the code.
>
> Wow, thanks.
Oy. Thanks for fixing my bug,
Dscho
>
> This probably indicates that nobody uses "rebase --signoff" in real
> life (i.e. where correct signed-off-by line matters). But still the
> bug is worth fixing.
>
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> > builtin/rebase.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index ddf33bc9d4..e354ec84bb 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -706,7 +706,7 @@ static int rebase_write_basic_state(struct rebase_options *opts)
> > write_file(state_dir_path("gpg_sign_opt", opts), "%s",
> > opts->gpg_sign_opt);
> > if (opts->signoff)
> > - write_file(state_dir_path("strategy", opts), "--signoff");
> > + write_file(state_dir_path("signoff", opts), "--signoff");
> >
> > return 0;
> > }
>
^ permalink raw reply
* Re: [PATCH 1/9] commit-graph: add --changed-paths option to write
From: Jakub Narebski @ 2020-01-01 20:20 UTC (permalink / raw)
To: Garima Singh via GitGitGadget
Cc: git, stolee, szeder.dev, jonathantanmy, jeffhost, me, peff,
Junio C Hamano, Garima Singh
In-Reply-To: <6bdde5e4f0ceb54546978e3e9cdde00045d45468.1576879520.git.gitgitgadget@gmail.com>
"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Garima Singh <garima.singh@microsoft.com>
>
> Add --changed-paths option to git commit-graph write. This option will
> soon allow users to compute bloom filters for the paths changed between
> a commit and its first significant parent, and write this information
> into the commit-graph file.
A slightly nitpicky comment.
First, I think it is "Bloom filter", not "bloom filter" (from the name
of the person that discovered them, Burton Howard Bloom).
Second, I would rather that the commit message started with at least one
sentence of describing purpose of this new option, not going straight to
the technical details (i.e. using Bloom filters). Or in any other way
describe that this option would make Git store some helper data that
would help find out faster if a given path was changed in given commit.
> Note: This commit does not change any behavior. It only introduces
> the option and passes down the appropriate flag to the commit-graph.
All right.
Personally, I don't have strong opinion for or against separating this
change into its own patch.
> RFC Notes:
> 1. We named the option --changed-paths to capture what the option does,
> instead of how it does it. The current implementation does this
> using bloom filters. We believe using --changed-paths however keeps
> the implementation open to other data structures.
> All thoughts and suggestions for the name and this approach are
> welcome
It is all right name. Another option could be for example
`git commit-graph write --changeset-info`, or something like that.
>
> 2. Currently, a subsequent commit in this series will add tests that
> exercise this option. I plan to split that test commit across the
> series as appropriate.
There is another thing, but one that could be left for the followup
series, namely the configuration variables for this behavior. In the
future it should be possible to switch some configuration variable to
have this feature on by default when manually or automatically running
`git commit-graph write`.
>
> Helped-by: Derrick Stolee <dstolee@microsoft.com>
> Signed-off-by: Garima Singh <garima.singh@microsoft.com>
> ---
> Documentation/git-commit-graph.txt | 5 +++++
> builtin/commit-graph.c | 9 +++++++--
> commit-graph.h | 3 ++-
> 3 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
> index bcd85c1976..1efe6e5c5a 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
It is nice to have option documented.
All right, the 'write' subcommand has the following synopsis:
'git commit-graph write' <options> [--object-dir <dir>] [--[no-]progress]
so the is no need to adjust it when adding a new option.
> @@ -54,6 +54,11 @@ or `--stdin-packs`.)
> With the `--append` option, include all commits that are present in the
> existing commit-graph file.
> +
> +With the `--changed-paths` option, compute and write information about the
> +paths changed between a commit and it's first parent. This operation can
> +take a while on large repositories. It provides significant performance gains
> +for getting file based history logs with `git log`
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This might be not entirely clear for someone that is not familiar with
Git jargon. Perhaps it would better read as "for getting history of a
directory or a file with `git log <path>`", or something like that.
Side note: the sentence is missing its finishing full stop.
> ++
> With the `--split` option, write the commit-graph as a chain of multiple
> commit-graph files stored in `<dir>/info/commit-graphs`. The new commits
> not already in the commit-graph are added in a new "tip" file. This file
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index e0c6fc4bbf..9bd1e11161 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -9,7 +9,7 @@
>
> static char const * const builtin_commit_graph_usage[] = {
> N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
> - N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
> + N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--changed-paths] [--[no-]progress] <split options>"),
> NULL
> };
>
> @@ -19,7 +19,7 @@ static const char * const builtin_commit_graph_verify_usage[] = {
> };
>
> static const char * const builtin_commit_graph_write_usage[] = {
> - N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
> + N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--changed-paths] [--[no-]progress] <split options>"),
> NULL
> };
I was at first wondering why the duplication (not caused by your patch,
though), and then realized that it is to have usage for command, and for
individual subcommands, separately.
> @@ -32,6 +32,7 @@ static struct opts_commit_graph {
> int split;
> int shallow;
> int progress;
> + int enable_bloom_filters;
Why the field is called `enable_bloom_filters`, while option is called
`--changed-paths`? I know it is not user-visible thing, so it would be
easy to change if we ever go beyond Bloom filters, though...
So I am not against keeping it as it is currently.
> } opts;
>
> static int graph_verify(int argc, const char **argv)
> @@ -110,6 +111,8 @@ static int graph_write(int argc, const char **argv)
> N_("start walk at commits listed by stdin")),
> OPT_BOOL(0, "append", &opts.append,
> N_("include all commits already in the commit-graph file")),
> + OPT_BOOL(0, "changed-paths", &opts.enable_bloom_filters,
> + N_("enable computation for changed paths")),
> OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
> OPT_BOOL(0, "split", &opts.split,
> N_("allow writing an incremental commit-graph file")),
> @@ -143,6 +146,8 @@ static int graph_write(int argc, const char **argv)
> flags |= COMMIT_GRAPH_WRITE_SPLIT;
> if (opts.progress)
> flags |= COMMIT_GRAPH_WRITE_PROGRESS;
> + if (opts.enable_bloom_filters)
> + flags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS;
Minor nitpick: are we all right having this ordering of options (not for
example having opt.progress last)?
Disregarding this, it looks all right.
>
> read_replace_refs = 0;
>
> diff --git a/commit-graph.h b/commit-graph.h
> index 7f5c933fa2..952a4b83be 100644
> --- a/commit-graph.h
> +++ b/commit-graph.h
> @@ -76,7 +76,8 @@ enum commit_graph_write_flags {
> COMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),
> COMMIT_GRAPH_WRITE_SPLIT = (1 << 2),
> /* Make sure that each OID in the input is a valid commit OID. */
> - COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3)
> + COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3),
> + COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 4)
I wonder if we should add comment describing the flag, like for the one
above...
> };
>
> struct split_commit_graph_opts {
Best,
--
Jakub Narębski
^ permalink raw reply
* [GSOC] Introduction
From: Hariom verma @ 2020-01-01 13:02 UTC (permalink / raw)
To: git
Hello,
I'm Hariom Verma, a third-year Computer Science Engineering Student.
I'm using Git for almost 2 years and want to contribute to the same. I had
gone through the solid part of 'SubmittingPatches' and 'Historical Summer of
Code Materials'.
I previously solved 2 issues[1][2] with the help of dscho and looking
forward to work on more before working on minor project to have a good
understanding of Git's Code. But thought I'd take a quick break to
introduce myself.
Thanks
Hariom Verma
[1]: https://github.com/gitgitgadget/git/issues/357
[2]: https://github.com/gitgitgadget/git/issues/399
^ permalink raw reply
* Re: [PATCH 0/9] [RFC] Changed Paths Bloom Filters
From: Jakub Narebski @ 2020-01-01 12:04 UTC (permalink / raw)
To: Jeff King
Cc: Christian Couder, Garima Singh via GitGitGadget, git,
Derrick Stolee, SZEDER Gábor, Jonathan Tan, Jeff Hostetler,
Taylor Blau, Junio C Hamano
In-Reply-To: <20191222093857.GB3449072@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sun, Dec 22, 2019 at 10:26:20AM +0100, Christian Couder wrote:
>
>> I have a question though. Are the performance gains only available
>> with `git log -- path` or are they already available for example when
>> doing a partial clone and/or a sparse checkout?
>
> From my quick look at the code, anything that feeds a pathspec to a
> revision traversal would be helped. I'm not sure if it would help for
> partial/sparse traversals, though. There we actually need to know which
> blobs correspond to the paths in question, not just whether any
> particular commit touched them.
>
> I also took a brief look at adding support to the custom blame-tree
> implementation we use at GitHub, and got about a 6x speedup.
Is there any chance of upstreaming the blame-tree algorithm, perhaps as
a separate mode for git-blame (invoked with `git blame <directory>`?
Or is the algorithm too GitHub-specific?
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 05/16] t2018: don't lose return code of git commands
From: Eric Sunshine @ 2020-01-01 9:21 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
In-Reply-To: <20200101084840.GC5152@generichostname>
On Wed, Jan 1, 2020 at 3:48 AM Denton Liu <liu.denton@gmail.com> wrote:
> On Fri, Dec 27, 2019 at 04:42:53PM -0500, Eric Sunshine wrote:
> > On Fri, Dec 27, 2019 at 8:48 AM Denton Liu <liu.denton@gmail.com> wrote:
> > > We had some git commands wrapped in non-assignment command substitutions
> > > which would result in their return codes to be lost. Rewrite these
> > > instances so that their return codes are now checked.
> >
> > Try writing your commit messages in imperative mood:
> >
> > Fix invocations of Git commands so their exit codes are not lost
> > within command substitutions...
>
> I thought that the preferred form of commit messages is to introduce the
> problem that I'm trying to solve first ("We had some git commands losing
> return codes") then, after that, describe the changes I made in
> imperative mood ("Rewrite these instances...").
My suggested rewrite includes both the problem ("exit codes ... lost
within command substitutions") and the change in one sentence. The
shorter you can make the commit message without omitting important
information and without being outright cryptic, the better. Let the
patch itself do the talking as much as possible.
Here's an example, from another patch in this series, which provides
too much information:
While we're at it, replace the test_might_fail(), which should
only be used on git commands, with a compound command that always
returns 0, even if the underlying ls fails.
The bit about "...with a compound command..." is unnecessary and
wastes reviewer time and is far more difficult to understand than
simply looking at the code in the patch itself. This would have been
sufficient:
While here, stop using test_might_fail() with 'ls' since it should
only be used with git commands.
> From what I can tell,
> all of my commit messages conform to this template.
> I'd prefer to keep the "problem statement" introduction in my commit
> messages as it primes readers so they know "why" before "what" but I
> can't think of any way to phrase the "problem statement" part in a way
> that sounds good without resorting to past tense. Any suggestions?
Using past tense makes it ambiguous to reviewers whether you're
talking about the state of the code which existed some time back or if
you're talking about the state of the code immediately before this
patch is applied. Try rewriting your commit messages without words
like "previously", "before", "had", "now", etc. For instance, the
commit message for this patch could be rewritten (keeping your wording
as much as possible) like this:
t2018: don't lose return code of git commands
The exit code of git commands wrapped in non-assignment command
substitutions is lost. Rewrite these invocations so the exit codes
are checked.
^ permalink raw reply
* Re: [PATCH 16/16] t4124: let sed open its own files
From: Denton Liu @ 2020-01-01 8:53 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Junio C Hamano, Jakub Narebski, Git Mailing List
In-Reply-To: <CAPig+cQWgH2MPZWZm1iL20at7BWN7-0p2=ogf8pHjGKvHDLL6g@mail.gmail.com>
Hi Eric,
On Wed, Jan 01, 2020 at 03:33:59AM -0500, Eric Sunshine wrote:
> (In fact, after reviewing this series -- and its
> predecessors -- I was strongly considering asking you to limit these
> cleanup series to 10 or fewer patches rather than 16, precisely due to
> feeling reviewer fatigue.)
Will do. You've been unbelievably helpful as the primary reviewer for
these cleanup series and I'd like to make sure that I don't cause you to
burn out.
Thanks for your patience,
Denton
^ permalink raw reply
* Re: [PATCH 05/16] t2018: don't lose return code of git commands
From: Denton Liu @ 2020-01-01 8:48 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git Mailing List
In-Reply-To: <CAPig+cQmO2W7kcqWZwrSsC7-vkk_UVcC5NktY+0dddcyaegr6Q@mail.gmail.com>
Hi Eric,
On Fri, Dec 27, 2019 at 04:42:53PM -0500, Eric Sunshine wrote:
> On Fri, Dec 27, 2019 at 8:48 AM Denton Liu <liu.denton@gmail.com> wrote:
> > We had some git commands wrapped in non-assignment command substitutions
> > which would result in their return codes to be lost. Rewrite these
> > instances so that their return codes are now checked.
>
> Try writing your commit messages in imperative mood:
>
> Fix invocations of Git commands so their exit codes are not lost
> within command substitutions...
>
> or something. Same comment about several other commit messages in this series.
I thought that the preferred form of commit messages is to introduce the
problem that I'm trying to solve first ("We had some git commands losing
return codes") then, after that, describe the changes I made in
imperative mood ("Rewrite these instances..."). From what I can tell,
all of my commit messages conform to this template.
I'd prefer to keep the "problem statement" introduction in my commit
messages as it primes readers so they know "why" before "what" but I
can't think of any way to phrase the "problem statement" part in a way
that sounds good without resorting to past tense. Any suggestions?
>
> More below...
>
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> > diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> > @@ -23,7 +23,8 @@ do_checkout () {
> > # if <sha> is not specified, use HEAD.
> > - exp_sha=${2:-$(git rev-parse --verify HEAD)} &&
> > + head=$(git rev-parse --verify HEAD) &&
> > + exp_sha=${2:-$head} &&
>
> Are you sure this transformation is needed? In my tests, the exit code
> of the Git command is not lost.
Thanks for double checking, it's not needed. I'll drop this in my next
reroll.
^ permalink raw reply
* Re: [PATCH 16/16] t4124: let sed open its own files
From: Eric Sunshine @ 2020-01-01 8:33 UTC (permalink / raw)
To: Denton Liu; +Cc: Junio C Hamano, Jakub Narebski, Git Mailing List
In-Reply-To: <20200101082443.GB5152@generichostname>
On Wed, Jan 1, 2020 at 3:25 AM Denton Liu <liu.denton@gmail.com> wrote:
> I assumed that letting these programs open their own input was one of
> those minor "style cleanup" things that was done such as removing
> spaces after redirection operators, indenting compound command blocks,
> etc. Whenever I've been cleaning tests up, if I noticed a `sed <` in the
> area, I'd apply the transformation as a style cleanup.
>
> Anyway, if no one else feels strongly about this, I can drop this patch
> and I'll stop doing this cleanup in the future.
As a reviewer, I'd just as soon not see that particular change which
increases reviewer burden yet provides no practical benefit. This is
especially true for these lengthy patch series which can easily lead
to reviewer fatigue. (In fact, after reviewing this series -- and its
predecessors -- I was strongly considering asking you to limit these
cleanup series to 10 or fewer patches rather than 16, precisely due to
feeling reviewer fatigue.)
^ permalink raw reply
* Re: [PATCH 16/16] t4124: let sed open its own files
From: Denton Liu @ 2020-01-01 8:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narebski, Git Mailing List
In-Reply-To: <xmqqfth1fl72.fsf@gitster-ct.c.googlers.com>
Hi Jakub and Junio,
On Mon, Dec 30, 2019 at 03:27:45PM -0800, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > Denton Liu <liu.denton@gmail.com> writes:
> >
> >> In one case, we were using a redirection operator to feed input into
> >> sed. However, since sed is capable of opening its own files, make sed
> >> open its own files instead of redirecting input into it.
> >
> > Could you please write in the commit message what advantages does this
> > change bring?
>
> A fair question.
>
> My version of short answer is "nothing---it is not wrong to write it
> either way, and it is not worth the patch churn to rewrite it from
> one form to the other, once the script is written".
In one of my earliest contributions to Git, Gábor suggested that since
since 'head' and 'sed' can open its own input file, there's no need to
redirect its standard input[1].
I assumed that letting these programs open their own input was one of
those minor "style cleanup" things that was done such as removing
spaces after redirection operators, indenting compound command blocks,
etc. Whenever I've been cleaning tests up, if I noticed a `sed <` in the
area, I'd apply the transformation as a style cleanup.
I guess the only tangible benefit I can think of is that programs which
have a file open can optimise on the fact that they can perform random
access on the file whereas with stdin, only linear access is allowed. I
don't know enough about the internals of various seds to be able to
comment on whether any of them actually take advantage of this, however.
Anyway, if no one else feels strongly about this, I can drop this patch
and I'll stop doing this cleanup in the future.
Thanks,
Denton
[1]: https://lore.kernel.org/git/20190317130539.GA23160@szeder.dev/
^ permalink raw reply
* Re: [PATCH v2 00/17] merge: learn --autostash
From: Denton Liu @ 2020-01-01 7:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, Alban Gruin, Johannes Schindelin, Phillip Wood
In-Reply-To: <xmqqo8vpfpri.fsf@gitster-ct.c.googlers.com>
Hi Junio,
On Mon, Dec 30, 2019 at 01:49:05PM -0800, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
>
> > Alban reported[1] that he expected merge to have an --autostash option,
> > just like rebase. Since there's not really any reason why merge can't
> > have it, let's implement it in this patchset.
>
> Sigh.
>
> I would actually have preferred to remove the autostash from rebase
> if the goal is to have a consistent behaviour between the two,
> though ;-)
>
> But it is OK as long as it does not degrade the codepath with
> changes that wouldn't have been necessary if this weren't added.
>
> Let's see how it goes. Queued.
Sorry for the radio silence for the past week. I've been offline trying
to recover from a particularly bad flu. Anyway, I have a reroll of this
topic planned with the `flags` change you suggested and addressing
Philip's concerns about --no-commit.
Since this is a new topic that definitely won't be accepted for v2.25.0,
I'll probably be taking my time with this ;)
Thanks,
Denton
^ permalink raw reply
* [PATCH 1/1] merge-recursive: remove unnecessary oid_eq function
From: Elijah Newren via GitGitGadget @ 2020-01-01 5:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Elijah Newren
In-Reply-To: <pull.685.git.git.1577856057.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
Back when merge-recursive was first introduced in commit 6d297f8137
(Status update on merge-recursive in C, 2006-07-08), it created a
sha_eq() function. This function pre-dated the introduction of
hashcmp() to cache.h by about a month, but was switched over to using
hashcmp() as part of commit 9047ebbc22 (Split out merge_recursive() to
merge-recursive.c, 2008-08-12). In commit b4da9d62f9 (merge-recursive:
convert leaf functions to use struct object_id, 2016-06-24), sha_eq() was
renamed to oid_eq() and its hashcmp() call was switched to oideq().
oid_eq() is basically just a wrapper around oideq() that has some extra
checks to protect against NULL arguments or to allow short-circuiting if
one of the arguments is NULL. I don't know if any caller ever tried to
call with NULL arguments, but certainly none do now which means the
extra checks serve no purpose. (Also, if these checks were genuinely
useful, then they probably should be added to the main oideq() so all
callers could benefit from them.)
Reduce the cognitive overhead of having both oid_eq() and oideq(), by
getting rid of merge-recursive's special oid_eq() wrapper.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
merge-recursive.c | 33 +++++++++++----------------------
1 file changed, 11 insertions(+), 22 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 11869ad81c..10dca5644b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -224,17 +224,6 @@ static struct commit *make_virtual_commit(struct repository *repo,
return commit;
}
-/*
- * Since we use get_tree_entry(), which does not put the read object into
- * the object pool, we cannot rely on a == b.
- */
-static int oid_eq(const struct object_id *a, const struct object_id *b)
-{
- if (!a && !b)
- return 2;
- return a && b && oideq(a, b);
-}
-
enum rename_type {
RENAME_NORMAL = 0,
RENAME_VIA_DIR,
@@ -805,7 +794,7 @@ static int was_tracked_and_matches(struct merge_options *opt, const char *path,
/* See if the file we were tracking before matches */
ce = opt->priv->orig_index.cache[pos];
- return (oid_eq(&ce->oid, &blob->oid) && ce->ce_mode == blob->mode);
+ return (oideq(&ce->oid, &blob->oid) && ce->ce_mode == blob->mode);
}
/*
@@ -1317,7 +1306,7 @@ static int merge_mode_and_contents(struct merge_options *opt,
oidcpy(&result->blob.oid, &b->oid);
}
} else {
- if (!oid_eq(&a->oid, &o->oid) && !oid_eq(&b->oid, &o->oid))
+ if (!oideq(&a->oid, &o->oid) && !oideq(&b->oid, &o->oid))
result->merge = 1;
/*
@@ -1333,9 +1322,9 @@ static int merge_mode_and_contents(struct merge_options *opt,
}
}
- if (oid_eq(&a->oid, &b->oid) || oid_eq(&a->oid, &o->oid))
+ if (oideq(&a->oid, &b->oid) || oideq(&a->oid, &o->oid))
oidcpy(&result->blob.oid, &b->oid);
- else if (oid_eq(&b->oid, &o->oid))
+ else if (oideq(&b->oid, &o->oid))
oidcpy(&result->blob.oid, &a->oid);
else if (S_ISREG(a->mode)) {
mmbuffer_t result_buf;
@@ -1368,7 +1357,7 @@ static int merge_mode_and_contents(struct merge_options *opt,
switch (opt->recursive_variant) {
case MERGE_VARIANT_NORMAL:
oidcpy(&result->blob.oid, &a->oid);
- if (!oid_eq(&a->oid, &b->oid))
+ if (!oideq(&a->oid, &b->oid))
result->clean = 0;
break;
case MERGE_VARIANT_OURS:
@@ -2836,15 +2825,15 @@ static int process_renames(struct merge_options *opt,
dst_other.mode = ren1->dst_entry->stages[other_stage].mode;
try_merge = 0;
- if (oid_eq(&src_other.oid, &null_oid) &&
+ if (oideq(&src_other.oid, &null_oid) &&
ren1->dir_rename_original_type == 'A') {
setup_rename_conflict_info(RENAME_VIA_DIR,
opt, ren1, NULL);
- } else if (oid_eq(&src_other.oid, &null_oid)) {
+ } else if (oideq(&src_other.oid, &null_oid)) {
setup_rename_conflict_info(RENAME_DELETE,
opt, ren1, NULL);
} else if ((dst_other.mode == ren1->pair->two->mode) &&
- oid_eq(&dst_other.oid, &ren1->pair->two->oid)) {
+ oideq(&dst_other.oid, &ren1->pair->two->oid)) {
/*
* Added file on the other side identical to
* the file being renamed: clean merge.
@@ -2859,7 +2848,7 @@ static int process_renames(struct merge_options *opt,
1, /* update_cache */
0 /* update_wd */))
clean_merge = -1;
- } else if (!oid_eq(&dst_other.oid, &null_oid)) {
+ } else if (!oideq(&dst_other.oid, &null_oid)) {
/*
* Probably not a clean merge, but it's
* premature to set clean_merge to 0 here,
@@ -3037,7 +3026,7 @@ static int blob_unchanged(struct merge_options *opt,
if (a->mode != o->mode)
return 0;
- if (oid_eq(&o->oid, &a->oid))
+ if (oideq(&o->oid, &a->oid))
return 1;
if (!renormalize)
return 0;
@@ -3478,7 +3467,7 @@ static int merge_trees_internal(struct merge_options *opt,
opt->subtree_shift);
}
- if (oid_eq(&merge_base->object.oid, &merge->object.oid)) {
+ if (oideq(&merge_base->object.oid, &merge->object.oid)) {
output(opt, 0, _("Already up to date!"));
*result = head;
return 1;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/1] merge-recursive: remove unnecessary oid_eq function
From: Elijah Newren via GitGitGadget @ 2020-01-01 5:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
merge-recursive has an unnecessary wrapper for oideq. Remove it.
Elijah Newren (1):
merge-recursive: remove unnecessary oid_eq function
merge-recursive.c | 33 +++++++++++----------------------
1 file changed, 11 insertions(+), 22 deletions(-)
base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-685%2Fnewren%2Fremove_oid_eq_wrapper-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-685/newren/remove_oid_eq_wrapper-v1
Pull-Request: https://github.com/git/git/pull/685
--
gitgitgadget
^ permalink raw reply
* Reduction in blanks in `git status` makes output pretty cramped
From: Anthony Sottile @ 2019-12-31 23:18 UTC (permalink / raw)
To: Git Mailing List
This is a comment on 7b098429355bb3271f9ffdf73b97f2ef82794fea
https://github.com/git/git/commit/7b098429355bb3271f9ffdf73b97f2ef82794fea
^ permalink raw reply
* [PATCH v2 1/1] mingw: only test index entries for backslashes, not tree entries
From: Johannes Schindelin via GitGitGadget @ 2019-12-31 22:53 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Junio C Hamano, Johannes Schindelin
In-Reply-To: <pull.682.v2.git.git.1577832830.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
During a clone of a repository that contained a file with a backslash in
its name in the past, as of v2.24.1(2), Git for Windows prints errors
like this:
error: filename in tree entry contains backslash: '\'
The idea is to prevent Git from even trying to write files with
backslashes in their file names: while these characters are valid in
file names on other platforms, on Windows it is interpreted as directory
separator (which would obviously lead to ambiguities, e.g. when there is
a file `a\b` and there is also a file `a/b`).
Arguably, this is the wrong layer for that error: As long as the user
never checks out the files whose names contain backslashes, there should
not be any problem in the first place.
So let's loosen the requirements: we now leave tree entries with
backslashes in their file names alone, but we do require any entries
that are added to the Git index to contain no backslashes on Windows.
Note: just as before, the check is guarded by `core.protectNTFS` (to
allow overriding the check by toggling that config setting), and it
is _only_ performed on Windows, as the backslash is not a directory
separator elsewhere, even when writing to NTFS-formatted volumes.
An alternative approach would be to try to prevent creating files with
backslashes in their file names. However, that comes with its own set of
problems. For example, `git config -f C:\ProgramData\Git\config ...` is
a very valid way to specify a custom config location, and we obviously
do _not_ want to prevent that. Therefore, the approach chosen in this
patch would appear to be better.
This addresses https://github.com/git-for-windows/git/issues/2435
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
read-cache.c | 5 +++++
t/t7415-submodule-names.sh | 7 ++++---
tree-walk.c | 6 ------
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index ad0b48c84d..737916ebd9 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1278,6 +1278,11 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
int skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;
int new_only = option & ADD_CACHE_NEW_ONLY;
+#ifdef GIT_WINDOWS_NATIVE
+ if (protect_ntfs && strchr(ce->name, '\\'))
+ return error(_("filename in tree entry contains backslash: '%s'"), ce->name);
+#endif
+
if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
cache_tree_invalidate_path(istate, ce->name);
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index 905a557585..7ae0dc8ff4 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -207,6 +207,9 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '
git hash-object -w --stdin)" &&
rev="$(git rev-parse --verify HEAD)" &&
hash="$(echo x | git hash-object -w --stdin)" &&
+ test_must_fail git update-index --add \
+ --cacheinfo 160000,$rev,d\\a 2>err &&
+ test_i18ngrep backslash err &&
git -c core.protectNTFS=false update-index --add \
--cacheinfo 100644,$modules,.gitmodules \
--cacheinfo 160000,$rev,c \
@@ -214,9 +217,7 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '
--cacheinfo 100644,$hash,d./a/x \
--cacheinfo 100644,$hash,d./a/..git &&
test_tick &&
- git -c core.protectNTFS=false commit -m "module" &&
- test_must_fail git show HEAD: 2>err &&
- test_i18ngrep backslash err
+ git -c core.protectNTFS=false commit -m "module"
) &&
test_must_fail git -c core.protectNTFS=false \
clone --recurse-submodules squatting squatting-clone 2>err &&
diff --git a/tree-walk.c b/tree-walk.c
index b3d162051f..d5a8e096a6 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -43,12 +43,6 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l
strbuf_addstr(err, _("empty filename in tree entry"));
return -1;
}
-#ifdef GIT_WINDOWS_NATIVE
- if (protect_ntfs && strchr(path, '\\')) {
- strbuf_addf(err, _("filename in tree entry contains backslash: '%s'"), path);
- return -1;
- }
-#endif
len = strlen(path) + 1;
/* Initialize the descriptor entry */
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/1] Disallow writing, but not fetching commits with file names containing backslashes
From: Johannes Schindelin via GitGitGadget @ 2019-12-31 22:53 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Junio C Hamano
In-Reply-To: <pull.682.git.git.1577382151.gitgitgadget@gmail.com>
As of Git for Windows, v2.24.1(2). cloning a repository that contained a
file with a backslash in its name some time in the past, the command will
succeed but at the same time print errors like this:
error: filename in tree entry contains backslash: '\'
A corresponding git fetch will also show those errors, but fail.
The reason is that v2.24.1 is much more strict about backslashes in tree
entries than earlier versions. The intention was actually to prevent
checking out such files, though: if there was a mistake in a repository long
ago that has been fixed long since, there is actually no reason why we
should require the history to be rewritten.
This fixes https://github.com/git-for-windows/git/issues/2435.
The idea of this patch is to stop complaining about tree entries, and focus
instead on the index: whenever a file is added to the index, we do not want
any backslashes in the file name on Windows.
As before, this check is only performed on Windows, and only under
core.protectNTFS. On other platforms, even if core.protectNTFS is turned on,
the backslash is not a directory separator, therefore the Operating System's
syscalls will (should?) refuse to create files on NTFS with backslashes in
their file name.
I would appreciate reviews with a particular eye on keeping users safe: I am
not 100% certain that all relevant file writes go through the index (I think
that they all go through the index, but I might have well missed a corner
case).
Changes since v1:
* Clarified the commit message (what is the goal, explain that the
requirement is now loosened, explain why the code is still
GIT_WINDOWS_NATIVE-only, etc).
Johannes Schindelin (1):
mingw: only test index entries for backslashes, not tree entries
read-cache.c | 5 +++++
t/t7415-submodule-names.sh | 7 ++++---
tree-walk.c | 6 ------
3 files changed, 9 insertions(+), 9 deletions(-)
base-commit: 12029dc57db23baef008e77db1909367599210ee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-682%2Fdscho%2Fmingw-only-error-on-backslash-in-index-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-682/dscho/mingw-only-error-on-backslash-in-index-v2
Pull-Request: https://github.com/git/git/pull/682
Range-diff vs v1:
1: 4a120fd0b3 ! 1: d6da8315d3 mingw: only test index entries for backslashes, not tree entries
@@ -8,14 +8,31 @@
error: filename in tree entry contains backslash: '\'
- While the clone still succeeds, a similar error prevents the equivalent
- `git fetch` operation, which is inconsistent.
+ The idea is to prevent Git from even trying to write files with
+ backslashes in their file names: while these characters are valid in
+ file names on other platforms, on Windows it is interpreted as directory
+ separator (which would obviously lead to ambiguities, e.g. when there is
+ a file `a\b` and there is also a file `a/b`).
- Arguably, this is the wrong layer for that error, anyway: As long as the
- user never checks out the files whose names contain backslashes, there
- should not be any problem in the first place.
+ Arguably, this is the wrong layer for that error: As long as the user
+ never checks out the files whose names contain backslashes, there should
+ not be any problem in the first place.
- So let's instead prevent such files to be added to the index.
+ So let's loosen the requirements: we now leave tree entries with
+ backslashes in their file names alone, but we do require any entries
+ that are added to the Git index to contain no backslashes on Windows.
+
+ Note: just as before, the check is guarded by `core.protectNTFS` (to
+ allow overriding the check by toggling that config setting), and it
+ is _only_ performed on Windows, as the backslash is not a directory
+ separator elsewhere, even when writing to NTFS-formatted volumes.
+
+ An alternative approach would be to try to prevent creating files with
+ backslashes in their file names. However, that comes with its own set of
+ problems. For example, `git config -f C:\ProgramData\Git\config ...` is
+ a very valid way to specify a custom config location, and we obviously
+ do _not_ want to prevent that. Therefore, the approach chosen in this
+ patch would appear to be better.
This addresses https://github.com/git-for-windows/git/issues/2435
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries
From: Johannes Schindelin @ 2019-12-31 22:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Johannes Schindelin via GitGitGadget, git
In-Reply-To: <xmqqblruk9lz.fsf@gitster-ct.c.googlers.com>
Hi Junio & Jonathan,
On Thu, 26 Dec 2019, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Jonathan Nieder <jrnieder@gmail.com> writes:
> >
> >> Is there anything we can or should do to prevent people checking in
> >> new examples of paths with backslash in them (on all platforms)?
> >
> > I obviously won't dictate what should happen on Windows, but I think
> > the overall principle for paths recorded in a tree object that can
> > be problematic on some of the platforms ought to be:
> >
> > * fsck and transfer.fsckobjects should be taught to notice
> > offending characteristics (e.g. has a backslash in it, is one of
> > the "reserved names" on some platform like LPT1).
Agree. This is on my radar, but so far not too-high priority, as the
`fsck` checks are not (yet?) standard practice in PR builds (and warnings
on the server side are prone to be ignored).
> > * if paths with the offending characteristics are *so* obviously
> > useless in real life and are possible only in a crafted path that
> > is only useful to attack users, the check in fsck should default
> > to "reject" to help the disease spread via hosting sites.
I don't think that reserved names such as `aux`, nor names containing
backslashes should be rejected _always_. While I cannot think of _any_
instance where I would want to have a backslash in a file name, I am sure
that just like `aux.c`, there _must_ be somebody out there who thought of
a file name that contains a backslash and makes at least some sort of
sense.
> > * otherwise, the check should be to "warn" but not "reject", so
> > that projects can keep using paths that may problematic on
> > platforms that do not matter to them.
Yes, it should be "warn".
> > I think LPT1 and friends fall into the "warning is fine" category,
> > and a path component that contains a backslash would fall into the
> > "this is an attack, just reject" category.
>
> I guess I should have stepped back a bit.
>
> In the message I am responding to, I focused solely on how tree
> objects that originate elsewhere should be treated, but there are
> two more data paths we need to worry about:
>
> * A new path gets introduced to the system, via "update-index",
> "add", etc., to the index and then "write-tree" to form tree
> objects in the local repository.
Right, that's what I had in mind when I wrote this patch. The path gets
added to the index, we detect a backslash, and on Windows (under
`core.protectNTFS`) fail with an error.
> * A path in the index, either created locally via "update-index",
> "add", etc., or read from a tree object, gets written to the
> local filesystem, resulting in an attempt to create a path that
> the local filesystem cannot represent (or worse---behaves badly,
> like "sending random garbage to the printer").
Happily, my patch seems to catch this code path, too: when reading from a
`tree` object into the index, we use `add_index_entry()` (called via
various code paths in the `unpack_trees()` machinery). That's exactly the
patched function.
Or maybe you know of a code path in the `unpack_trees()` machinery that
does _not_ go through `add_index_entry()`? I would be very interested to
learn about such code paths.
> I think we should apply the same principle as the one I outlined for
> the tree objects. The fsckobjects mechanism may not be reusable to
> catch violations in add_index_entry_with_check() as-is, but we need
> to aim for as much reuse of logic and code as possible so that our
> checks at various layers all behave consistently.
I am afraid that we won't be able to reuse code paths for checking the
backslash here, but for reserved names I am planning on refactoring the
code accordingly.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH v2 1/1] commit: display advice hints when commit fails
From: Junio C Hamano @ 2019-12-31 19:06 UTC (permalink / raw)
To: Jonathan Tan; +Cc: gitgitgadget, git, heba.waly
In-Reply-To: <20191231000420.32396-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
>> In any case, here is what I tentatively have in my tree (with heavy
>> rewrite to the proposed log message).
>
> Junio, what are your plans over what you have in your tree? If you'd
> like to hear Heba's opinion on it, then she can chime in; if you'd like
> a review, then I think it's good to go in.
On hold until anything like those happens ;-)
A random reviewer mentioning something on a patch (either in a
line-by-line critique form or "how about doing it this way instead"
counterproposal form) without getting followed up by others
(including the original author) is a stall review thread, and it
does not change the equation if the random reviewer happens to be me.
>> I didn't try it on my end. Maybe it won't help much, because we think
>> we're going to use the editor right up until we realize it's not
>> committable?
>
> And I think the answer to that is "s" is used throughout the function in
> various ways (in particular, used to print statuses both to stdout and
> to the message template) so any wrapping or corralling of scope would
> just make things more complicated. In particular, the way Heba did it in
> v2 is more unclear - at the time of setting s->hints = 0, it's done
You mean "less clear" (just double checking if I got the negation right)?
> within a "if (use_editor && include_status)" block, but (as far as I can
> tell) the commit message template might also be used when there is no
> editor - for example, as input to a hook. And more importantly, when
> s->hints is reset to the config, we don't know at that point that the
> next status is going to stdout. So I think it's better just to use the
> v1 way.
Yeah, thanks for going back to compare v1 and v2, and I agree with
your assessment.
> The second area of discussion I see is in the commit message. Commit
> messages have to balance brevity and comprehensiveness, and this can be
> a subjective matter, but I think Junio's strikes a good balance.
As one side of the comparison is my own, I won't be a good judge on
this, but yes I tried to strick a good balance as much as possible.
I think I've merged it to 'next' yesterday, but it does not mean
that much as we are in -rc and it is not such an urgent "oops we
broke it in this cycle, let's fix it" issue. If we see a v3 that
improves it, I do not mind at all reverting what I merged to 'next'
and use the updated one instead (either way, it will be in 'master'
during the next cycle at the earliest).
Thanks.
^ permalink raw reply
* Re: [PATCH 0/9] [RFC] Changed Paths Bloom Filters
From: Jakub Narebski @ 2019-12-31 16:45 UTC (permalink / raw)
To: Garima Singh via GitGitGadget
Cc: git, stolee, szeder.dev, jonathantanmy, jeffhost, me, peff,
Junio C Hamano
In-Reply-To: <pull.497.git.1576879520.gitgitgadget@gmail.com>
"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Hey!
>
> The commit graph feature brought in a lot of performance improvements across
> multiple commands. However, file based history continues to be a performance
> pain point, especially in large repositories.
>
> Adopting changed path bloom filters has been discussed on the list before,
> and a prototype version was worked on by SZEDER Gábor, Jonathan Tan and Dr.
> Derrick Stolee [1]. This series is based on Dr. Stolee's approach [2] and
> presents an updated and more polished RFC version of the feature.
It is nice to have this picked up for upstream, finally. The proof of
concept works[1][2] were started more than a year ago.
On the other hand slow and steady adoption of commit-graph serialization
and then extending it (generation numbers, topological sort, incremental
update) feels like a good approach.
> Performance Gains: We tested the performance of 'git log -- <path>' on the git
> repo, the linux repo and some internal large repos, with a variety of paths
> of varying depths.
>
> On the git and linux repos: We observed a 2x to 5x speed up.
>
> On a large internal repo with files seated 6-10 levels deep in the tree: We
> observed 10x to 20x speed ups, with some paths going up to 28 times faster.
Could you provide some more statistics about this internal repository,
such as number of files, number of commits, perhaps also number of all
objects? Thanks in advance.
I wonder why such large difference in performance 2-5x vs 10-20x. Is it
about the depth of the file hierarchy? How would the numbers look for
files seated closer to the root in the same large repository, like 3-5
levels deep in the tree?
> Future Work (not included in the scope of this series):
>
> 1. Supporting multiple path based revision walk
I wonder if it would ever be possible to support globbing, e.g. '*.c'
> 2. Adopting it in git blame logic.
What about 'git log --follow <path>'?
> 3. Interactions with line log git log -L
>
> This series is intended to start the conversation and many of the commit
> messages include specific call outs for suggestions and thoughts.
>
> Cheers! Garima Singh
>
> [1] https://lore.kernel.org/git/20181009193445.21908-1-szeder.dev@gmail.com/
> [2] https://lore.kernel.org/git/61559c5b-546e-d61b-d2e1-68de692f5972@gmail.com/
>
> Garima Singh (9):
> commit-graph: add --changed-paths option to write
This summary is not easy to understand on first glance. Maybe:
commit-graph: add --changed-paths option to the write subcommand
or
commit-graph: add --changed-paths option to 'git commit-graph write'
would be better?
> commit-graph: write changed paths bloom filters
> commit-graph: use MAX_NUM_CHUNKS
> commit-graph: document bloom filter format
> commit-graph: write changed path bloom filters to commit-graph file.
> commit-graph: test commit-graph write --changed-paths
> commit-graph: reuse existing bloom filters during write.
> revision.c: use bloom filters to speed up path based revision walks
> commit-graph: add GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS test flag
>
> Documentation/git-commit-graph.txt | 5 +
> .../technical/commit-graph-format.txt | 17 ++
> Makefile | 1 +
> bloom.c | 257 +++++++++++++++++
> bloom.h | 51 ++++
> builtin/commit-graph.c | 9 +-
> ci/run-build-and-tests.sh | 1 +
> commit-graph.c | 116 +++++++-
> commit-graph.h | 9 +-
> revision.c | 67 ++++-
> revision.h | 5 +
> t/README | 3 +
> t/helper/test-read-graph.c | 4 +
> t/t4216-log-bloom.sh | 77 ++++++
> t/t5318-commit-graph.sh | 2 +
> t/t5324-split-commit-graph.sh | 1 +
> t/t5325-commit-graph-bloom.sh | 258 ++++++++++++++++++
> 17 files changed, 875 insertions(+), 8 deletions(-)
> create mode 100644 bloom.c
> create mode 100644 bloom.h
> create mode 100755 t/t4216-log-bloom.sh
> create mode 100755 t/t5325-commit-graph-bloom.sh
>
>
> base-commit: b02fd2accad4d48078671adf38fe5b5976d77304
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-497%2Fgarimasi514%2FcoreGit-bloomFilters-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-497/garimasi514/coreGit-bloomFilters-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/497
^ permalink raw reply
* [PATCH 1/1] sparse-checkout: use extern for global variables
From: Derrick Stolee via GitGitGadget @ 2019-12-31 13:17 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.506.git.1577798268.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
When the core.sparseCheckoutCone config setting was added in
879321eb0b ("sparse-checkout: add 'cone' mode" 2019-11-21), the
variables storing the config values for core.sparseCheckout and
core.sparseCheckoutCone were rearranged in cache.h, but in doing
so the "extern" keyword was dropped.
While we are tending to drop the "extern" keyword for function
declarations, it is still necessary for global variables used
across multiple *.c files. The impact of not having the extern
keyword may be unpredictable.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
cache.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 1554488d66..cbfaead23a 100644
--- a/cache.h
+++ b/cache.h
@@ -958,8 +958,8 @@ extern int protect_hfs;
extern int protect_ntfs;
extern const char *core_fsmonitor;
-int core_apply_sparse_checkout;
-int core_sparse_checkout_cone;
+extern int core_apply_sparse_checkout;
+extern int core_sparse_checkout_cone;
/*
* Include broken refs in all ref iterations, which will
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/1] sparse-checkout: use extern for global variables
From: Derrick Stolee via GitGitGadget @ 2019-12-31 13:17 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Junio C Hamano
I noticed this issue when resolving conflicts with our VFS for Git-enabled
branch in microsoft/git.
When I moved the global for core.sparseCheckout along with creating the
global for core.sparseCheckoutCone, I dropped the "extern" by habit. (We are
dropping these from function declarations, usually.) However, this means
something different for variables, and could lead to bugs. I haven't found
any, but it's better to be safe, right?
Thanks, -Stolee
Derrick Stolee (1):
sparse-checkout: use extern for global variables
cache.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
base-commit: 99c33bed562b41de6ce9bd3fd561303d39645048
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-506%2Fderrickstolee%2Fsparse-extern-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-506/derrickstolee/sparse-extern-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/506
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v2 00/17] merge: learn --autostash
From: Phillip Wood @ 2019-12-31 10:34 UTC (permalink / raw)
To: Junio C Hamano, Denton Liu
Cc: Git Mailing List, Alban Gruin, Johannes Schindelin
In-Reply-To: <xmqqo8vpfpri.fsf@gitster-ct.c.googlers.com>
On 30/12/2019 21:49, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
>
>> Alban reported[1] that he expected merge to have an --autostash option,
>> just like rebase. Since there's not really any reason why merge can't
>> have it, let's implement it in this patchset.
>
> Sigh.
>
> I would actually have preferred to remove the autostash from rebase
> if the goal is to have a consistent behaviour between the two,
> though ;-)
I find the --autostash option to rebase useful if I want to edit a
commit and I've got uncommitted changes. I wish it was called --stash
though as it's not automatic as one has to set a config setting or pass
it on the commandline. I'm also not convinced the config setting is a
good idea as it allows rebase to run when I think I've committed
everything but I haven't.
For `merge --autostash` I think we need to consider the interaction of
`--no-commit` with `--autostash` as `stash pop` will refuse to run if
the merge modified any of the stashed paths.
Best Wishes
Phillip
> But it is OK as long as it does not degrade the codepath with
> changes that wouldn't have been necessary if this weren't added.
>
> Let's see how it goes. Queued.
>
^ permalink raw reply
* [PATCH v4 3/3] t: drop copy&pasted tests for --pathspec-from-file
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-31 10:15 UTC (permalink / raw)
To: git
Cc: Jonathan Nieder, Eric Sunshine, Alexandr Miloslavskiy,
Junio C Hamano, Alexandr Miloslavskiy
In-Reply-To: <pull.503.v4.git.1577787313.gitgitgadget@gmail.com>
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
With direct tests for `parse_pathspec_file()` already in place, it is
not very reasonable to copy&paste 6 similar indirect tests for every git
command that uses `parse_pathspec_file()`. I counted 13 potential git
commands, which could eventually lead to 6*13=78 duplicate tests.
I believe that indirect tests are redundant because I don't expect
direct tests to ever disagree with indirect tests.
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
t/t2026-checkout-pathspec-file.sh | 77 +---------------------------
t/t2072-restore-pathspec-file.sh | 77 +---------------------------
t/t3704-add-pathspec-file.sh | 77 +---------------------------
t/t7107-reset-pathspec-file.sh | 85 +++----------------------------
t/t7526-commit-pathspec-file.sh | 77 +---------------------------
5 files changed, 16 insertions(+), 377 deletions(-)
diff --git a/t/t2026-checkout-pathspec-file.sh b/t/t2026-checkout-pathspec-file.sh
index adad71f631..559b4528d7 100755
--- a/t/t2026-checkout-pathspec-file.sh
+++ b/t/t2026-checkout-pathspec-file.sh
@@ -35,7 +35,7 @@ verify_expect () {
test_cmp expect actual
}
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
restore_checkpoint &&
echo fileA.t | git checkout --pathspec-from-file=- HEAD^1 &&
@@ -46,19 +46,7 @@ test_expect_success '--pathspec-from-file from stdin' '
verify_expect
'
-test_expect_success '--pathspec-from-file from file' '
- restore_checkpoint &&
-
- echo fileA.t >list &&
- git checkout --pathspec-from-file=list HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'NUL delimiters' '
+test_expect_success '--pathspec-file-nul' '
restore_checkpoint &&
printf "fileA.t\0fileB.t\0" | git checkout --pathspec-from-file=- --pathspec-file-nul HEAD^1 &&
@@ -70,67 +58,6 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t\n" | git checkout --pathspec-from-file=- HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- M fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t" | git checkout --pathspec-from-file=- HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- M fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git checkout --pathspec-from-file=- HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- M fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- git checkout --pathspec-from-file=list HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- test_must_fail git checkout --pathspec-from-file=list --pathspec-file-nul HEAD^1
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
diff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh
index b407f6b779..9b3125d582 100755
--- a/t/t2072-restore-pathspec-file.sh
+++ b/t/t2072-restore-pathspec-file.sh
@@ -35,7 +35,7 @@ verify_expect () {
test_cmp expect actual
}
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
restore_checkpoint &&
echo fileA.t | git restore --pathspec-from-file=- --source=HEAD^1 &&
@@ -46,19 +46,7 @@ test_expect_success '--pathspec-from-file from stdin' '
verify_expect
'
-test_expect_success '--pathspec-from-file from file' '
- restore_checkpoint &&
-
- echo fileA.t >list &&
- git restore --pathspec-from-file=list --source=HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'NUL delimiters' '
+test_expect_success '--pathspec-file-nul' '
restore_checkpoint &&
printf "fileA.t\0fileB.t\0" | git restore --pathspec-from-file=- --pathspec-file-nul --source=HEAD^1 &&
@@ -70,67 +58,6 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t\n" | git restore --pathspec-from-file=- --source=HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- M fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t" | git restore --pathspec-from-file=- --source=HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- M fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git restore --pathspec-from-file=- --source=HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- M fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- git restore --pathspec-from-file=list --source=HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- test_must_fail git restore --pathspec-from-file=list --pathspec-file-nul --source=HEAD^1
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
diff --git a/t/t3704-add-pathspec-file.sh b/t/t3704-add-pathspec-file.sh
index 61b6e51009..9009f8a9ac 100755
--- a/t/t3704-add-pathspec-file.sh
+++ b/t/t3704-add-pathspec-file.sh
@@ -23,7 +23,7 @@ verify_expect () {
test_cmp expect actual
}
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
restore_checkpoint &&
echo fileA.t | git add --pathspec-from-file=- &&
@@ -34,19 +34,7 @@ test_expect_success '--pathspec-from-file from stdin' '
verify_expect
'
-test_expect_success '--pathspec-from-file from file' '
- restore_checkpoint &&
-
- echo fileA.t >list &&
- git add --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'NUL delimiters' '
+test_expect_success '--pathspec-file-nul' '
restore_checkpoint &&
printf "fileA.t\0fileB.t\0" | git add --pathspec-from-file=- --pathspec-file-nul &&
@@ -58,67 +46,6 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t\n" | git add --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t" | git add --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git add --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- git add --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- test_must_fail git add --pathspec-from-file=list --pathspec-file-nul
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
diff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh
index b0e84cdb42..5b845f4f7c 100755
--- a/t/t7107-reset-pathspec-file.sh
+++ b/t/t7107-reset-pathspec-file.sh
@@ -25,7 +25,7 @@ verify_expect () {
test_cmp expect actual
}
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
restore_checkpoint &&
git rm fileA.t &&
@@ -37,20 +37,7 @@ test_expect_success '--pathspec-from-file from stdin' '
verify_expect
'
-test_expect_success '--pathspec-from-file from file' '
- restore_checkpoint &&
-
- git rm fileA.t &&
- echo fileA.t >list &&
- git reset --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'NUL delimiters' '
+test_expect_success '--pathspec-file-nul' '
restore_checkpoint &&
git rm fileA.t fileB.t &&
@@ -63,77 +50,21 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- git rm fileA.t fileB.t &&
- printf "fileA.t\nfileB.t\n" | git reset --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- D fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- git rm fileA.t fileB.t &&
- printf "fileA.t\nfileB.t" | git reset --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- D fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
+test_expect_success 'only touches what was listed' '
restore_checkpoint &&
- git rm fileA.t fileB.t &&
- printf "fileA.t\r\nfileB.t\r\n" | git reset --pathspec-from-file=- &&
+ git rm fileA.t fileB.t fileC.t fileD.t &&
+ printf "fileB.t\nfileC.t\n" | git reset --pathspec-from-file=- &&
cat >expect <<-\EOF &&
- D fileA.t
+ D fileA.t
D fileB.t
+ D fileC.t
+ D fileD.t
EOF
verify_expect
'
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- git rm fileA.t &&
- git reset --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- # Note: "git reset" has not yet learned to fail on wrong pathspecs
- git reset --pathspec-from-file=list --pathspec-file-nul &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- EOF
- test_must_fail verify_expect
-'
-
test_expect_success '--pathspec-from-file is not compatible with --soft or --hard' '
restore_checkpoint &&
diff --git a/t/t7526-commit-pathspec-file.sh b/t/t7526-commit-pathspec-file.sh
index 4a7c11368d..8d6c652690 100755
--- a/t/t7526-commit-pathspec-file.sh
+++ b/t/t7526-commit-pathspec-file.sh
@@ -26,7 +26,7 @@ verify_expect () {
test_cmp expect actual
}
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
restore_checkpoint &&
echo fileA.t | git commit --pathspec-from-file=- -m "Commit" &&
@@ -37,19 +37,7 @@ test_expect_success '--pathspec-from-file from stdin' '
verify_expect
'
-test_expect_success '--pathspec-from-file from file' '
- restore_checkpoint &&
-
- echo fileA.t >list &&
- git commit --pathspec-from-file=list -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'NUL delimiters' '
+test_expect_success '--pathspec-file-nul' '
restore_checkpoint &&
printf "fileA.t\0fileB.t\0" | git commit --pathspec-from-file=- --pathspec-file-nul -m "Commit" &&
@@ -61,67 +49,6 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t\n" | git commit --pathspec-from-file=- -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t" | git commit --pathspec-from-file=- -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git commit --pathspec-from-file=- -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- git commit --pathspec-from-file=list -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- EOF
- verify_expect expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- test_must_fail git commit --pathspec-from-file=list --pathspec-file-nul -m "Commit"
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
--
gitgitgadget
^ 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