Re: [PATCH] travis-ci: no longer use containers
Junio C Hamano writes:
>
> Sebastian Staudt writes:
>
> > Travis CI will soon deprecate the container-based infrastructure
> > enabled by `sudo: false` in ce59dffb34190e780be2fa9f449f842cadee9753.
> >
> > More info:
> > https://blog.travis-ci.com/2018-10-04-combining-linux-infrastructures
>
> Thanks for posting a patch that would serve as a good discussion
> starter. This is not a criticism on your patch, but more is a RFD
> to those who helped our use of Travis by contributing to .travis.yml
> and ci/.
In fact this was the intention while creating this patch. Although I see
I could have made this a bit clearer in the initial message.
Having a patch that may cause a broken build or other CI problems
seems more appropriate than waiting for Travis CI to flip the switch
and searching for the problem afterwards.
> Don't we need to do some other things so that we can run in vm
> environment, rather than in container environment, before doing this
> change? IOW, aren't we doing in .travis.yml something we can do
> only in container but not in vm (if there is any), and if so,
> shouldn't we be rewriting that something so that we can run in vm?
>
> I know ce59dffb ("travis-ci: explicity use container-based
> infrastructure", 2016-01-26) only added "sudo: false" without doing
> anything else (e.g. adding things that are only available to those
> who run in container), but if we added stuff that are not usable in
> vm environment after that commit since then, we need to adjust them
> so that we can migrate to the container-based environment, no?
>
> To me, removing that "sudo: false" line seems like the least thing
> we need to worry about. After all, they say that whether we have
> "sudo: false" or not, the CI jobs will start running in vm
> environment and not in container. So if the rest of .travis.yml is
> ready to run in vm environment, we do not have to do anything ;-).
>
> In short, my question to Lars and SZEDER is, are we already prepared
> to be thrown into a vm environment?
>
> If the answer is "yes", then I think removing "sudo: false" is
> probably still a good thing to do for documentation purposes
> (i.e. showing that we knew we are ready to go through their
> migration).
>
> Thanks.
>
> > Signed-off-by: Sebastian Staudt
> > ---
> > .travis.yml | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/.travis.yml b/.travis.yml
> > index 4d4e26c9df..8d2499739e 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -1,7 +1,5 @@
> > language: c
> >
> > -sudo: false
> > -
> > cache:
> >directories:
> > - $HOME/travis-cache
> > --
> > 2.19.1
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
Johannes Schindelin writes: >> I did not and I do not think it would. I was wondering if the >> ability to be able to specify these per destination is something >> very useful and deserves to be called out in the doc, together with >> ... > > I do not think that it needs to be called out specifically in the docs. It > is just yet another http.* setting that can be overridden per-URL. It > would be different if it had not worked. OK, thanks for sanity checking.
Re: [PATCH] http: give curl version warnings consistently
Johannes Schindelin writes:
> On Thu, 25 Oct 2018, Junio C Hamano wrote:
>
>> When a requested feature cannot be activated because the version of
>> cURL library used to build Git with is too old, most of the codepaths
>> give a warning like "$Feature is not supported with cURL < $Version",
>> marked for l10n. A few of them, however, did not follow that pattern
>> and said things like "$Feature is not activated, your curl version is
>> too old (>= $Version)", and without marking them for l10n.
>>
>> Update these to match the style of the majority of warnings and mark
>> them for l10n.
>>
>> Signed-off-by: Junio C Hamano
>> ---
>
> I like this patch better than the one I had prepared for v2, so I dropped
> it again, and "hit the Submit button".
I took your v3 and queue this on top, instead of the previous one
on which this was prepared.
By the way, I wondered if we want to unify them by introducing
static void curl_version_warning(const char *feature, const char
*verstring)
{
warning(_("%s is not supported with cURL < %s"),
feature, verstring);
}
so that translators need to deal with a single instance of the
message, but the "feature" part may have to get localized, in which
case we'd end up with sentence lego, which is not a good idea, so I
dropped it.
Thanks.
Re: [PATCH v3 3/3] commit-slab: missing definitions and forward declarations (hdr-check)
On Thu, Oct 25, 2018 at 2:09 PM Ramsay Jones wrote: > Yes, this will 'fix' the 'commit-reach.h' header (not surprising), > but I prefer my patch. ;-) I apologize, I joined the list recently and so might had missed a reroll; the merged series in pu doesn't seem to include it and the error was around the code I changed, so wanted to make sure it would be addressed sooner rather than later. eitherway, I agree with you my patch (or something better) would fit better in your topic branch than on mine and while I haven't seen your patch I am sure is most likely better. > Still puzzled. this are the last lines of a `make hdr-check` in Fedora Rawhide, it should behave the same regardless of OS or compiler used IMHO HDR commit-reach.h commit-reach.h:45:28: warning: ‘struct object_id’ declared inside parameter list will not be visible outside of this definition or declaration int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid); ^ In file included from commit-slab.h:5, from commit-reach.h:4: commit-reach.h: In function ‘contains_cache_at_peek’: commit-slab-impl.h:47:14: error: dereferencing pointer to incomplete type ‘const struct commit’ nth_slab = c->index / s->slab_size;\ ^~ commit-slab-impl.h:7:2: note: in expansion of macro ‘implement_commit_slab’ implement_commit_slab(slabname, elemtype, static MAYBE_UNUSED) ^ commit-slab.h:49:2: note: in expansion of macro ‘implement_static_commit_slab’ implement_static_commit_slab(slabname, elemtype) ^~~~ commit-reach.h:57:1: note: in expansion of macro ‘define_commit_slab’ define_commit_slab(contains_cache, enum contains_result); ^~ commit-reach.h: At top level: commit-reach.h:69:41: warning: ‘struct object_array’ declared inside parameter list will not be visible outside of this definition or declaration int can_all_from_reach_with_flag(struct object_array *from, ^~~~ make: *** [Makefile:2685: commit-reach.hco] Error 1 Carlo
Re: [PATCH v1 2/2] curl_off_t xcurl_off_t is not portable
tbo...@web.de writes:
> From: Torsten Bögershausen
> Subject: Re: [PATCH v1 2/2] curl_off_t xcurl_off_t is not portable
That title is misleading; it sounded as if the are these two
typedefs and they do not work correctly on some platforms, but that
is not what you are doing with the patch.
> Comparing signed and unsigned values is not always portable.
Is that what the compiler is complaining about? There is this bit
in git-compat-util.h:
/*
* Signed integer overflow is undefined in C, so here's a helper macro
* to detect if the sum of two integers will overflow.
*
* Requires: a >= 0, typeof(a) equals typeof(b)
*/
#define signed_add_overflows(a, b) \
((b) > maximum_signed_value_of_type(a) - (a))
which is designed to be fed signed a and signed b. The macro is
used in packfile codepaths to compare int, off_t, etc..
So the statement may be true but it does not seem to have much to do
with the problem you are seeing with maximum_signed_value_of_type().
> When setting
> DEVELOPER = 1
> DEVOPTS = extra-all
>
> "gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516" errors out with
> "comparison is always false due to limited range of data type"
> "[-Werror=type-limits]"
Then this sounds a bit different from "comparison between signed
ssize_t len and unsigned maximum_signed_value_of_type() is bad".
Isn't it saying that "No matter how big you make len, you can never
go beyond maximum_signed_value_of_type(curl_off_t)"?
> diff --git a/remote-curl.c b/remote-curl.c
> index 762a55a75f..c89fd6d1c3 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -618,9 +618,10 @@ static int probe_rpc(struct rpc_state *rpc, struct
> slot_results *results)
> }
>
> static curl_off_t xcurl_off_t(ssize_t len) {
> - if (len > maximum_signed_value_of_type(curl_off_t))
Is the issue that len is signed and maximum_signed_value_of_type()
gives an unsigned value, and these two are compared? As we saw
earlier, signed_add_overflows() is another example that wants a
mixed comparison.
I am just wondering if casting len to uintmax_t before comparing
with maximum_signed_value_of_type() is a simpler solution that can
safely be cargo-culted to other places without much thinking.
"git grep maximum_signed_value_of_type" reports a handful
comparisons in vcs-svn/, all of which does
if (var > maximum_signed_value_of_type(off_t))
with var of type uintmax_t, which sounds like a sane thing to do.
Thanks.
> + curl_off_t size = (curl_off_t) len;
> + if (len != (ssize_t) size)
> die("cannot handle pushes this big");
> - return (curl_off_t) len;
> + return size;
> }
Re: [PATCH v7 00/10] Make submodules work if .gitmodules is not checked out
Stefan Beller writes: >> In this series I am addressing the comments by Stefan Beller about the >> tests in patch 9. >> >> If the new tests look OK, I'd say we try moving the series to "next" and >> see what happens? > > Sounds good to me. Which means (1) the plan sounds OK but I didn't look at these new tests or (2) the new tests look OK and I am happy to see this go to 'next'? tbdiff tells me that 9/10 is the only patch different from the previous round, and I vaguely recall that the other patches looked OK to me (even though I admit I only skimmed them quickly). Thanks.
Re: [PATCH v5] branch: introduce --show-current display option
Eric Sunshine writes: >> + test_when_finished " >> + git checkout branch-one >> + git branch -D branch-and-tag-name >> + " && >> + git checkout -b branch-and-tag-name && >> + test_when_finished "git tag -d branch-and-tag-name" && >> + git tag branch-and-tag-name && We've discussed about the exit status from clean-up code already, but another thing worth noticing is that it probably is easier to see what is going on if we use a single when-finished to clear both branch and the tag with the same name. Something like test_when_finished " git checkout branch-one git branch -D branch-and-tag-name git tag -d branch-and-tag-name : " && upfront before doing anything else. "checkout" may break if the test that follows when-finished accidentally removes branch-one and that would cascade to a failure to remove branch-and-tag-name branch (because we fail to move away from it), but because there is no && in between, we'd clean as much as we could in such a case, which may or may not be a good thing. And then we hide the exit code by having a ":" at the end.
Re: [PATCH] travis-ci: no longer use containers
On Fri, Oct 26, 2018 at 09:09:48AM +0900, Junio C Hamano wrote:
> Sebastian Staudt writes:
>
> > Travis CI will soon deprecate the container-based infrastructure
> > enabled by `sudo: false` in ce59dffb34190e780be2fa9f449f842cadee9753.
> >
> > More info:
> > https://blog.travis-ci.com/2018-10-04-combining-linux-infrastructures
>
> Thanks for posting a patch that would serve as a good discussion
> starter. This is not a criticism on your patch, but more is a RFD
> to those who helped our use of Travis by contributing to .travis.yml
> and ci/.
>
> Don't we need to do some other things so that we can run in vm
> environment, rather than in container environment, before doing this
> change? IOW, aren't we doing in .travis.yml something we can do
> only in container but not in vm (if there is any), and if so,
> shouldn't we be rewriting that something so that we can run in vm?
As far as I understand, the container-based infrastructure has only
one benefit over the VMs, the shorter startup time.
OTOH, in VMs we can use sudo, which is not available in the
container-based intra. This has the benefit that after switching to
VMs, we'll be able to install packages by running 'sudo apt-get
install ...'. Currently the necessary packages are listed in
'.travis.yml' for Travis CI, while for Azure the whole install command
is embedded in '.azureyml'. After the switch we could consolidate
installing packages by 'sudo apt-get...' in
'ci/install-dependencies.sh' for both.
> I know ce59dffb ("travis-ci: explicity use container-based
> infrastructure", 2016-01-26) only added "sudo: false" without doing
> anything else (e.g. adding things that are only available to those
> who run in container), but if we added stuff that are not usable in
> vm environment after that commit since then, we need to adjust them
> so that we can migrate to the container-based environment, no?
>
> To me, removing that "sudo: false" line seems like the least thing
> we need to worry about. After all, they say that whether we have
> "sudo: false" or not, the CI jobs will start running in vm
> environment and not in container. So if the rest of .travis.yml is
> ready to run in vm environment, we do not have to do anything ;-).
>
> In short, my question to Lars and SZEDER is, are we already prepared
> to be thrown into a vm environment?
I think we are. I've run only two builds with this patch, and they
run smoothly and finished successfully. After you update 'pu' I'll
run more.
> If the answer is "yes", then I think removing "sudo: false" is
> probably still a good thing to do for documentation purposes
> (i.e. showing that we knew we are ready to go through their
> migration).
I agree.
> > Signed-off-by: Sebastian Staudt
> > ---
> > .travis.yml | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/.travis.yml b/.travis.yml
> > index 4d4e26c9df..8d2499739e 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -1,7 +1,5 @@
> > language: c
> >
> > -sudo: false
> > -
> > cache:
> >directories:
> > - $HOME/travis-cache
> > --
> > 2.19.1
Re: [PATCH 3/2] rebase -i: recognize short commands without arguments
Johannes Sixt writes:
> The sequencer instruction 'b', short for 'break', is rejected:
>
> error: invalid line 2: b
>
> The reason is that the parser expects all short commands to have
> an argument. Permit short commands without arguments.
>
> Signed-off-by: Johannes Sixt
> ---
> I'll send a another patch in a moment that tests all short
> sequencer commands, but it is independent from this topic.
>
> sequencer.c| 3 ++-
> t/lib-rebase.sh| 2 +-
> t/t3418-rebase-continue.sh | 4 +++-
> 3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index ee3961ec63..3107f59ea7 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1954,7 +1954,8 @@ static int parse_insn_line(struct todo_item *item,
> const char *bol, char *eol)
> if (skip_prefix(bol, todo_command_info[i].str, )) {
> item->command = i;
> break;
> - } else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
> + } else if ((bol + 1 == eol || bol[1] == ' ') &&
> +*bol == todo_command_info[i].c) {
> bol++;
> item->command = i;
> break;
> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> index 584604ee63..86572438ec 100644
> --- a/t/lib-rebase.sh
> +++ b/t/lib-rebase.sh
> @@ -49,7 +49,7 @@ set_fake_editor () {
> case $line in
> squash|fixup|edit|reword|drop)
> action="$line";;
> - exec*|break)
> + exec*|break|b)
> echo "$line" | sed 's/_/ /g' >> "$1";;
> "#")
> echo '# comment' >> "$1";;
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 185a491089..b282505aac 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -243,7 +243,9 @@ unset GIT_SEQUENCE_EDITOR
>
> test_expect_success 'the todo command "break" works' '
> rm -f execed &&
> - FAKE_LINES="break exec_>execed" git rebase -i HEAD &&
> + FAKE_LINES="break b exec_>execed" git rebase -i HEAD &&
> + test_path_is_missing execed &&
When first 'break' hits, "git rebase -i" shouldn't have exited with
non-zero, and we get to see if execed is there (it shouldn't exist
yet).
> + git rebase --continue &&
And then we continue, to hit the next 'b', which shouldn't barf,
either, and then
> test_path_is_missing execed &&
we make sure execed is not yet there, before continuing out of that
that 'b'roken state
> git rebase --continue &&
and then we'll hit the exec to create that file.
> test_path_is_file execed
Makes sense. Thanks.
Re: [PATCH v5] archive: initialize archivers earlier
Jeff King writes:
> On Thu, Oct 25, 2018 at 01:32:14PM -0700, stead...@google.com wrote:
>
>> Initialize archivers as soon as possible when running git-archive.
>> Various non-obvious behavior depends on having the archivers
>> initialized, such as determining the desired archival format from the
>> provided filename.
>>
>> Since 08716b3c11 ("archive: refactor file extension format-guessing",
>> 2011-06-21), archive_format_from_filename() has used the registered
>> archivers to match filenames (provided via --output) to archival
>> formats. However, when git-archive is executed with --remote, format
>> detection happens before the archivers have been registered. This causes
>> archives from remotes to always be generated as TAR files, regardless of
>> the actual filename (unless an explicit --format is provided).
>>
>> This patch fixes that behavior; archival format is determined properly
>> from the output filename, even when --remote is used.
>
> Thanks, this version looks great to me!
To me too. Thanks, both.
Re: [PATCH v3 1/3] [Outreachy] t3903-stash: test without configured user name
Slavica Djukic writes: > From: Slavica Please make sure this matches your sign-off below. > This is part of enhancement request that ask for 'git stash' to work > even if 'user.name' and 'user.email' are not configured. > Due to an implementation detail, git-stash undesirably requires > 'user.name' and 'user.email' to be set, but shouldn't. > The issue is discussed here: > https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u. As the four lines above summarize the issue being highlighted by the expect-failure rather well, the last two lines are unnecessary. Please remove them. Alternatively, you can place them after the three-dash lines we see below. > Signed-off-by: Slavica Djukic > --- > t/t3903-stash.sh | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 9e06494ba0..ae2c905343 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -1156,4 +1156,18 @@ test_expect_success 'stash -- works with > binary files' ' > test_path_is_file subdir/untracked > ' > > +test_expect_failure 'stash works when user.name and user.email are not set' ' > +test_commit 1 && Just being curious, but do we need a fresh commit created at this point in the test? Many tests before this one begin with "git reset" and then run "git stash" without ever creating commit themselves, instead relying on the fact that there already is at least one commit created in the "setup" phase of the test that a "stash" created can be made relative to. I do not think this test is all that special in that regard to require its own commit. > +test_config user.useconfigonly true && > +test_config stash.usebuiltin true && > +sane_unset GIT_AUTHOR_NAME && > +sane_unset GIT_AUTHOR_EMAIL && > +sane_unset GIT_COMMITTER_NAME && > +sane_unset GIT_COMMITTER_EMAIL && > +test_unconfig user.email && There are trailing whitespaces on the line above. Please remove. Also, Don't be original in the form alone---all other tests in this file indent with a leading HT, not four SPs. Please match the style of surrounding code. > +test_unconfig user.name && > +echo changed >1.t && > +git stash > +' > + > test_done Thanks. Please do not reroll the next round at too rapid a pace.
Re: [PATCH v5] branch: introduce --show-current display option
Eric Sunshine writes: >> + test_when_finished "git tag -d branch-and-tag-name" && >> + git tag branch-and-tag-name && > > If git-tag crashes before actually creating the new tag, then "git tag > -d", passed to test_when_finished(), will error out too, which is > probably undesirable since "cleanup code" isn't expected to error out. Ah, I somehow thought that clean-up actions set up via when_finished are allowed to fail without affecting the outcome, but apparently I was mistaken. This however can be argued both ways---if you create a tag first and try to set up the clean-up action, during which you may get in trouble and end up leaving the tag behind. So rather than swapping the two lines, explicitly preparing for the case the clean-up action fails, i.e. the first alternative below, would be a good fix. Also it is a good question if the tag need to be even cleaned up. > You could fix it this way: > > test_when_finished "git tag -d branch-and-tag-name || :" && > git tag branch-and-tag-name && > > or, even better, just swap the two lines: > > git tag branch-and-tag-name && > test_when_finished "git tag -d branch-and-tag-name" && > However, do you even need to clean up the tag? Are there tests > following this one which expect a certain set of tags and fail if this > new one is present? If not, a simpler approach might be just to leave > the tag alone (and the branch too if that doesn't need to be cleaned > up). > >> + git branch --show-current >actual && >> + test_cmp expect actual >> +' A bigger question we may want to ask ourselves is if we want to detect failures from these clean-up actions in the first place. There are many hits from "git grep 'when_finished .*|| :' t/", which may be a sign that the when_finished mechanism was misdesigned and we should simply ignore the exit status from the clean-up actions instead. I haven't gone through the list of when_finished clean-up actions that do not end with "|| :"; I suspect some of them are simply being sloppy and would want to have "|| :", but what I want to find out out of such an audit is if there is a legitimate case where it helps to catch failures in the clean-up actions. If there is none, then ...
Re: [RFC PATCH] index-pack: improve performance on NFS
"Jansen, Geert" writes:
> The index-pack command determines if a sha1 collision test is needed by
> checking the existence of a loose object with the given hash. In my tests, I
> can improve performance of “git clone” on Amazon EFS by 8x when used with a
> non-default mount option (lookupcache=pos) that's required for a Gitlab HA
> setup. My assumption is that this check is unnecessary when cloning into a new
> repository because the repository will be empty.
My knee-jerk reaction is that your insight that we can skip the "dup
check" when starting from emptiness is probably correct, but your
use of .cloning flag as an approximation of "are we starting from
emptiness?" is probably wrong, at least for two reasons.
- "git clone --reference=..." does not strictly start from
emptiness, and would still have to make sure that incoming pack
does not try to inject an object with different contents but with
the same name.
- "git init && git fetch ..." starts from emptiness and would want
to benefit from the same optimization as you are implementing
here.
As to the implementation, I think the patch adjusts the right "if()"
condition to skip the collision test here.
> - if (startup_info->have_repository) {
> + if (startup_info->have_repository && !cloning) {
> read_lock();
> collision_test_needed =
> has_sha1_file_with_flags(oid->hash, OBJECT_INFO_QUICK);
I just do not think !cloning is quite correct.
Thanks.
Re: [PATCH] travis-ci: no longer use containers
Sebastian Staudt writes:
> Travis CI will soon deprecate the container-based infrastructure
> enabled by `sudo: false` in ce59dffb34190e780be2fa9f449f842cadee9753.
>
> More info:
> https://blog.travis-ci.com/2018-10-04-combining-linux-infrastructures
Thanks for posting a patch that would serve as a good discussion
starter. This is not a criticism on your patch, but more is a RFD
to those who helped our use of Travis by contributing to .travis.yml
and ci/.
Don't we need to do some other things so that we can run in vm
environment, rather than in container environment, before doing this
change? IOW, aren't we doing in .travis.yml something we can do
only in container but not in vm (if there is any), and if so,
shouldn't we be rewriting that something so that we can run in vm?
I know ce59dffb ("travis-ci: explicity use container-based
infrastructure", 2016-01-26) only added "sudo: false" without doing
anything else (e.g. adding things that are only available to those
who run in container), but if we added stuff that are not usable in
vm environment after that commit since then, we need to adjust them
so that we can migrate to the container-based environment, no?
To me, removing that "sudo: false" line seems like the least thing
we need to worry about. After all, they say that whether we have
"sudo: false" or not, the CI jobs will start running in vm
environment and not in container. So if the rest of .travis.yml is
ready to run in vm environment, we do not have to do anything ;-).
In short, my question to Lars and SZEDER is, are we already prepared
to be thrown into a vm environment?
If the answer is "yes", then I think removing "sudo: false" is
probably still a good thing to do for documentation purposes
(i.e. showing that we knew we are ready to go through their
migration).
Thanks.
> Signed-off-by: Sebastian Staudt
> ---
> .travis.yml | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/.travis.yml b/.travis.yml
> index 4d4e26c9df..8d2499739e 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -1,7 +1,5 @@
> language: c
>
> -sudo: false
> -
> cache:
>directories:
> - $HOME/travis-cache
> --
> 2.19.1
[PATCH v2] list-objects.c: don't segfault for missing cmdline objects
When a command is invoked with both --exclude-promisor-objects,
--objects-edge-aggressive, and a missing object on the command line,
the rev_info.cmdline array could get a NULL pointer for the value of
an 'item' field. Prevent dereferencing of a NULL pointer in that
situation.
There are a few other places in the code where rev_info.cmdline is read
and the code doesn't handle NULL objects, but I couldn't prove to myself
that any of them needed to change except this one (since it may not
actually be possible to reach the other code paths with
rev_info.cmdline[] set to NULL).
Signed-off-by: Matthew DeVore
---
list-objects.c | 3 ++-
t/t0410-partial-clone.sh | 6 +-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/list-objects.c b/list-objects.c
index c41cc80db5..27ed2c6cab 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -245,7 +245,8 @@ void mark_edges_uninteresting(struct rev_info *revs,
show_edge_fn show_edge)
for (i = 0; i < revs->cmdline.nr; i++) {
struct object *obj = revs->cmdline.rev[i].item;
struct commit *commit = (struct commit *)obj;
- if (obj->type != OBJ_COMMIT || !(obj->flags &
UNINTERESTING))
+ if (!obj || obj->type != OBJ_COMMIT ||
+ !(obj->flags & UNINTERESTING))
continue;
mark_tree_uninteresting(revs->repo,
get_commit_tree(commit));
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index ba3887f178..e52291e674 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -366,7 +366,11 @@ test_expect_success 'rev-list accepts missing and promised
objects on command li
git -C repo config core.repositoryformatversion 1 &&
git -C repo config extensions.partialclone "arbitrary string" &&
- git -C repo rev-list --exclude-promisor-objects --objects "$COMMIT"
"$TREE" "$BLOB"
+
+ git -C repo rev-list --objects \
+ --exclude-promisor-objects "$COMMIT" "$TREE" "$BLOB" &&
+ git -C repo rev-list --objects-edge-aggressive \
+ --exclude-promisor-objects "$COMMIT" "$TREE" "$BLOB"
'
test_expect_success 'gc repacks promisor objects separately from non-promisor
objects' '
--
2.19.1.568.g152ad8e336-goog
Re: [PATCH] Clear --exclude list after 'git rev-parse --all'
Jeff King writes: > An easy test is: > > $ git rev-list --no-walk --exclude='*' --all --all > 3289ca716320457af5d2dd550b716282ad22da11 > ...a bunch of other tip commits... > > $ git rev-parse --exclude='*' --all --all > [no output, but it should print those same tip commits] I actually was hoping to see a test that contrasts "--all" (which lacks the alleged "clear exclude" bug) with another option that does have the "clear exclude", both used with rev-parse, i.e. $ git rev-parse --exclude='*' --glob='*' --glob='*' ... all the ref tips ... $ git rev-parse --exclude='*' --all --all ... ought to be equivalent, but is empty due to the bug ... would have been a good demonstration that shows what bug we are fixing d(and would have been a good test to accompany the patch.
[PATCH 10/10] builtin/fetch: check for submodule updates in any ref update
Gerrit, the code review tool, has a different workflow than our mailing
list based approach. Usually users upload changes to a Gerrit server and
continuous integration and testing happens by bots. Sometimes however a
user wants to checkout a change locally and look at it locally. For this
use case, Gerrit offers a command line snippet to copy and paste to your
terminal, which looks like
git fetch https:///gerrit refs/changes/ &&
git checkout FETCH_HEAD
For Gerrit changes that contain changing submodule gitlinks, it would be
easy to extend both the fetch and checkout with the '--recurse-submodules'
flag, such that this command line snippet would produce the state of a
change locally.
However the functionality added in the previous patch, which would
ensure that we fetch the objects in the submodule that the gitlink pointed
at, only works for remote tracking branches so far, not for FETCH_HEAD.
Make sure that fetching a superproject to its FETCH_HEAD, also respects
the existence checks for objects in the submodule recursion.
Signed-off-by: Stefan Beller
---
builtin/fetch.c | 8 ++--
t/t5526-fetch-submodules.sh | 24
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 95c44bf6ff..f39012d7c2 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -700,8 +700,6 @@ static int update_local_ref(struct ref *ref,
what = _("[new ref]");
}
- if (recurse_submodules != RECURSE_SUBMODULES_OFF)
- check_for_new_submodule_commits(>new_oid);
r = s_update_ref(msg, ref, 0);
format_display(display, r ? '!' : '*', what,
r ? _("unable to update local ref") : NULL,
@@ -715,8 +713,6 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid,
DEFAULT_ABBREV);
strbuf_addstr(, "..");
strbuf_add_unique_abbrev(, >new_oid,
DEFAULT_ABBREV);
- if (recurse_submodules != RECURSE_SUBMODULES_OFF)
- check_for_new_submodule_commits(>new_oid);
r = s_update_ref("fast-forward", ref, 1);
format_display(display, r ? '!' : ' ', quickref.buf,
r ? _("unable to update local ref") : NULL,
@@ -729,8 +725,6 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid,
DEFAULT_ABBREV);
strbuf_addstr(, "...");
strbuf_add_unique_abbrev(, >new_oid,
DEFAULT_ABBREV);
- if (recurse_submodules != RECURSE_SUBMODULES_OFF)
- check_for_new_submodule_commits(>new_oid);
r = s_update_ref("forced-update", ref, 1);
format_display(display, r ? '!' : '+', quickref.buf,
r ? _("unable to update local ref") : _("forced
update"),
@@ -826,6 +820,8 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
ref->force = rm->peer_ref->force;
}
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
+ check_for_new_submodule_commits(>old_oid);
if (!strcmp(rm->name, "HEAD")) {
kind = "";
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 5a75b57852..799785783f 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -631,4 +631,28 @@ test_expect_success "fetch new submodule commits on-demand
outside standard refs
)
'
+test_expect_success 'fetch new submodule commits on-demand in FETCH_HEAD' '
+ # depends on the previous test for setup
+
+ C=$(git -C submodule commit-tree -m "another change outside refs/heads"
HEAD^{tree}) &&
+ git -C submodule update-ref refs/changes/1 $C &&
+ git update-index --cacheinfo 16 $C submodule &&
+ test_tick &&
+
+ D=$(git -C sub1 commit-tree -m "another change outside refs/heads"
HEAD^{tree}) &&
+ git -C sub1 update-ref refs/changes/2 $D &&
+ git update-index --cacheinfo 16 $D sub1 &&
+
+ git commit -m "updated submodules outside of refs/heads" &&
+ E=$(git rev-parse HEAD) &&
+ git update-ref refs/changes/2 $E &&
+ (
+ cd downstream &&
+ git fetch --recurse-submodules origin refs/changes/2 &&
+ git -C submodule cat-file -t $C &&
+ git -C sub1 cat-file -t $D &&
+ git checkout --recurse-submodules FETCH_HEAD
+ )
+'
+
test_done
--
2.19.0
[PATCH 09/10] fetch: try fetching submodules if needed objects were not fetched
Currently when git-fetch is asked to recurse into submodules, it dispatches
a plain "git-fetch -C " (with some submodule related options
such as prefix and recusing strategy, but) without any information of the
remote or the tip that should be fetched.
But this default fetch is not sufficient, as a newly fetched commit in
the superproject could point to a commit in the submodule that is not
in the default refspec. This is common in workflows like Gerrit's.
When fetching a Gerrit change under review (from refs/changes/??), the
commits in that change likely point to submodule commits that have not
been merged to a branch yet.
Try fetching a submodule by object id if the object id that the
superproject points to, cannot be found.
The try does not happen when the "git fetch" done at the
superproject is not storing the fetched results in remote
tracking branches (i.e. instead just recording them to
FETCH_HEAD) in this step. A later patch will fix this.
builtin/fetch used to only inspect submodules when they were fetched
"on-demand", as in either on/off case it was clear whether the submodule
needs to be fetched. However to know whether we need to try fetching the
object ids, we need to identify the object names, which is done in this
function check_for_new_submodule_commits(), so we'll also run that code
in case the submodule recursion is set to "on".
Signed-off-by: Stefan Beller
---
builtin/fetch.c | 9 +-
submodule.c | 192 ++--
t/t5526-fetch-submodules.sh | 31 ++
3 files changed, 198 insertions(+), 34 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 61bec5d213..95c44bf6ff 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -700,8 +700,7 @@ static int update_local_ref(struct ref *ref,
what = _("[new ref]");
}
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref(msg, ref, 0);
format_display(display, r ? '!' : '*', what,
@@ -716,8 +715,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid,
DEFAULT_ABBREV);
strbuf_addstr(, "..");
strbuf_add_unique_abbrev(, >new_oid,
DEFAULT_ABBREV);
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref("fast-forward", ref, 1);
format_display(display, r ? '!' : ' ', quickref.buf,
@@ -731,8 +729,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid,
DEFAULT_ABBREV);
strbuf_addstr(, "...");
strbuf_add_unique_abbrev(, >new_oid,
DEFAULT_ABBREV);
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref("forced-update", ref, 1);
format_display(display, r ? '!' : '+', quickref.buf,
diff --git a/submodule.c b/submodule.c
index 67813fbe78..c978a38c81 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1218,8 +1218,12 @@ struct submodule_parallel_fetch {
int result;
struct string_list changed_submodule_names;
+ struct get_next_submodule_task **fetch_specific_oids;
+ int fetch_specific_oids_nr, fetch_specific_oids_alloc;
};
-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0,
STRING_LIST_INIT_DUP }
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
+ STRING_LIST_INIT_DUP, \
+ NULL, 0, 0}
static int get_fetch_recurse_config(const struct submodule *submodule,
struct submodule_parallel_fetch *spf)
@@ -1246,6 +1250,58 @@ static int get_fetch_recurse_config(const struct
submodule *submodule,
return spf->default_option;
}
+struct get_next_submodule_task {
+ struct repository *repo;
+ const struct submodule *sub;
+ unsigned free_sub : 1; /* Do we need to free the submodule? */
+
+ /* fetch specific oids if set, otherwise fetch default refspec */
+ struct oid_array *commits;
+};
+
+static const struct submodule *get_default_submodule(const char *path)
+{
+ struct submodule *ret = NULL;
+ const char *name = default_name_or_path(path);
+
+ if (!name)
+ return NULL;
+
+ ret = xmalloc(sizeof(*ret));
+ memset(ret, 0,
[PATCH 08/10] submodule.c: fetch in submodules git directory instead of in worktree
Keep the properties introduced in 10f5c52656 (submodule: avoid
auto-discovery in prepare_submodule_repo_env(), 2016-09-01), by fixating
the git directory of the submodule.
Signed-off-by: Stefan Beller
---
submodule.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/submodule.c b/submodule.c
index a1a32cab7d..67813fbe78 100644
--- a/submodule.c
+++ b/submodule.c
@@ -494,6 +494,12 @@ void prepare_submodule_repo_env(struct argv_array *out)
DEFAULT_GIT_DIR_ENVIRONMENT);
}
+static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)
+{
+ prepare_submodule_repo_env_no_git_dir(out);
+ argv_array_pushf(out, "%s=.", GIT_DIR_ENVIRONMENT);
+}
+
/* Helper function to display the submodule header line prior to the full
* summary output. If it can locate the submodule objects directory it will
* attempt to lookup both the left and right commits and put them into the
@@ -1313,8 +1319,8 @@ static int get_next_submodule(struct child_process *cp,
repo = get_submodule_repo_for(spf->r, submodule);
if (repo) {
child_process_init(cp);
- prepare_submodule_repo_env(>env_array);
- cp->dir = xstrdup(repo->worktree);
+ prepare_submodule_repo_env_in_gitdir(>env_array);
+ cp->dir = xstrdup(repo->gitdir);
cp->git_cmd = 1;
if (!spf->quiet)
strbuf_addf(err, "Fetching submodule %s%s\n",
--
2.19.0
[PATCH 07/10] submodule: migrate get_next_submodule to use repository structs
We used to recurse into submodules, even if they were broken having
only an objects directory. The child process executed in the submodule
would fail though if the submodule was broken.
This patch tightens the check upfront, such that we do not need
to spawn a child process to find out if the submodule is broken.
Signed-off-by: Stefan Beller
---
submodule.c | 51 +++
1 file changed, 39 insertions(+), 12 deletions(-)
diff --git a/submodule.c b/submodule.c
index e4b494af7b..a1a32cab7d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1240,6 +1240,29 @@ static int get_fetch_recurse_config(const struct
submodule *submodule,
return spf->default_option;
}
+static struct repository *get_submodule_repo_for(struct repository *r,
+const struct submodule *sub)
+{
+ struct repository *ret = xmalloc(sizeof(*ret));
+
+ if (repo_submodule_init(ret, r, sub)) {
+ /*
+* No entry in .gitmodules? Technically not a submodule,
+* but historically we supported repositories that happen to be
+* in-place where a gitlink is. Keep supporting them.
+*/
+ struct strbuf gitdir = STRBUF_INIT;
+ strbuf_repo_worktree_path(, r, "%s/.git", sub->path);
+ if (repo_init(ret, gitdir.buf, NULL)) {
+ strbuf_release();
+ return NULL;
+ }
+ strbuf_release();
+ }
+
+ return ret;
+}
+
static int get_next_submodule(struct child_process *cp,
struct strbuf *err, void *data, void **task_cb)
{
@@ -1247,12 +1270,11 @@ static int get_next_submodule(struct child_process *cp,
struct submodule_parallel_fetch *spf = data;
for (; spf->count < spf->r->index->cache_nr; spf->count++) {
- struct strbuf submodule_path = STRBUF_INIT;
- struct strbuf submodule_git_dir = STRBUF_INIT;
struct strbuf submodule_prefix = STRBUF_INIT;
const struct cache_entry *ce = spf->r->index->cache[spf->count];
- const char *git_dir, *default_argv;
+ const char *default_argv;
const struct submodule *submodule;
+ struct repository *repo;
struct submodule default_submodule = SUBMODULE_INIT;
if (!S_ISGITLINK(ce->ce_mode))
@@ -1287,16 +1309,12 @@ static int get_next_submodule(struct child_process *cp,
continue;
}
- strbuf_repo_worktree_path(_path, spf->r, "%s",
ce->name);
- strbuf_addf(_git_dir, "%s/.git", submodule_path.buf);
strbuf_addf(_prefix, "%s%s/", spf->prefix, ce->name);
- git_dir = read_gitfile(submodule_git_dir.buf);
- if (!git_dir)
- git_dir = submodule_git_dir.buf;
- if (is_directory(git_dir)) {
+ repo = get_submodule_repo_for(spf->r, submodule);
+ if (repo) {
child_process_init(cp);
- cp->dir = strbuf_detach(_path, NULL);
prepare_submodule_repo_env(>env_array);
+ cp->dir = xstrdup(repo->worktree);
cp->git_cmd = 1;
if (!spf->quiet)
strbuf_addf(err, "Fetching submodule %s%s\n",
@@ -1306,10 +1324,19 @@ static int get_next_submodule(struct child_process *cp,
argv_array_push(>args, default_argv);
argv_array_push(>args, "--submodule-prefix");
argv_array_push(>args, submodule_prefix.buf);
+
+ repo_clear(repo);
+ free(repo);
ret = 1;
+ } else {
+ /*
+* An empty directory is normal,
+* the submodule is not initialized
+*/
+ if (S_ISGITLINK(ce->ce_mode) &&
+ !is_empty_dir(ce->name))
+ die(_("Could not access submodule '%s'"),
ce->name);
}
- strbuf_release(_path);
- strbuf_release(_git_dir);
strbuf_release(_prefix);
if (ret) {
spf->count++;
--
2.19.0
[PATCH 05/10] submodule: store OIDs in changed_submodule_names
'calculate_changed_submodule_paths' uses a local list to compute the
changed submodules, and then produces the result by copying appropriate
items into the result list.
Instead use the result list directly and prune items afterwards
using string_list_remove_empty_items.
By doing so we'll have access to the util pointer for longer that
contains the commits that we need to fetch, which will be
useful in a later patch.
Signed-off-by: Stefan Beller
Reviewed-by: Jonathan Tan
---
submodule.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/submodule.c b/submodule.c
index 6fb0b9d783..e4b494af7b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1127,8 +1127,7 @@ static void calculate_changed_submodule_paths(
struct string_list *changed_submodule_names)
{
struct argv_array argv = ARGV_ARRAY_INIT;
- struct string_list changed_submodules = STRING_LIST_INIT_DUP;
- const struct string_list_item *name;
+ struct string_list_item *name;
/* No need to check if there are no submodules configured */
if (!submodule_from_path(the_repository, NULL, NULL))
@@ -1145,9 +1144,9 @@ static void calculate_changed_submodule_paths(
* Collect all submodules (whether checked out or not) for which new
* commits have been recorded upstream in "changed_submodule_names".
*/
- collect_changed_submodules(_submodules, );
+ collect_changed_submodules(changed_submodule_names, );
- for_each_string_list_item(name, _submodules) {
+ for_each_string_list_item(name, changed_submodule_names) {
struct oid_array *commits = name->util;
const struct submodule *submodule;
const char *path = NULL;
@@ -1161,12 +1160,14 @@ static void calculate_changed_submodule_paths(
if (!path)
continue;
- if (!submodule_has_commits(path, commits))
- string_list_append(changed_submodule_names,
- name->string);
+ if (submodule_has_commits(path, commits)) {
+ oid_array_clear(commits);
+ *name->string = '\0';
+ }
}
- free_submodules_oids(_submodules);
+ string_list_remove_empty_items(changed_submodule_names, 1);
+
argv_array_clear();
oid_array_clear(_tips_before_fetch);
oid_array_clear(_tips_after_fetch);
@@ -1376,7 +1377,7 @@ int fetch_populated_submodules(struct repository *r,
argv_array_clear();
out:
- string_list_clear(_submodule_names, 1);
+ free_submodules_oids(_submodule_names);
return spf.result;
}
--
2.19.0
[PATCH 06/10] repository: repo_submodule_init to take a submodule struct
When constructing a struct repository for a submodule for some revision
of the superproject where the submodule is not contained in the index,
it may not be present in the working tree currently either. In that
situation giving a 'path' argument is not useful. Upgrade the
repo_submodule_init function to take a struct submodule instead.
The submodule struct can be obtained via submodule_from_{path, name} or
an artificial submodule struct can be passed in.
While we are at it, rename the repository struct in the repo_submodule_init
function, which is to be initialized, to a name that is not confused with
the struct submodule as easily. Perform such renames in similar functions
as well.
Also move its documentation into the header file.
Signed-off-by: Stefan Beller
---
builtin/grep.c | 17 +++-
builtin/ls-files.c | 12 +
builtin/submodule--helper.c | 2 +-
repository.c | 27
repository.h | 12 +++--
t/helper/test-submodule-nested-repo-config.c | 8 +++---
6 files changed, 43 insertions(+), 35 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 7da8fef31a..ba7634258a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -418,7 +418,10 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
const struct object_id *oid,
const char *filename, const char *path)
{
- struct repository submodule;
+ struct repository subrepo;
+ const struct submodule *sub = submodule_from_path(superproject,
+ _oid, path);
+
int hit;
/*
@@ -434,12 +437,12 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
return 0;
}
- if (repo_submodule_init(, superproject, path)) {
+ if (repo_submodule_init(, superproject, sub)) {
grep_read_unlock();
return 0;
}
- repo_read_gitmodules();
+ repo_read_gitmodules();
/*
* NEEDSWORK: This adds the submodule's object directory to the list of
@@ -451,7 +454,7 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
* store is no longer global and instead is a member of the repository
* object.
*/
- add_to_alternates_memory(submodule.objects->objectdir);
+ add_to_alternates_memory(subrepo.objects->objectdir);
grep_read_unlock();
if (oid) {
@@ -476,14 +479,14 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
init_tree_desc(, data, size);
hit = grep_tree(opt, pathspec, , , base.len,
- object->type == OBJ_COMMIT, );
+ object->type == OBJ_COMMIT, );
strbuf_release();
free(data);
} else {
- hit = grep_cache(opt, , pathspec, 1);
+ hit = grep_cache(opt, , pathspec, 1);
}
- repo_clear();
+ repo_clear();
return hit;
}
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 7f9919a362..4d1649c1b3 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -206,17 +206,19 @@ static void show_files(struct repository *repo, struct
dir_struct *dir);
static void show_submodule(struct repository *superproject,
struct dir_struct *dir, const char *path)
{
- struct repository submodule;
+ struct repository subrepo;
+ const struct submodule *sub = submodule_from_path(superproject,
+ _oid, path);
- if (repo_submodule_init(, superproject, path))
+ if (repo_submodule_init(, superproject, sub))
return;
- if (repo_read_index() < 0)
+ if (repo_read_index() < 0)
die("index file corrupt");
- show_files(, dir);
+ show_files(, dir);
- repo_clear();
+ repo_clear();
}
static void show_ce(struct repository *repo, struct dir_struct *dir,
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5f8a804a6e..015aa1471f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2037,7 +2037,7 @@ static int ensure_core_worktree(int argc, const char
**argv, const char *prefix)
if (!sub)
BUG("We could get the submodule handle before?");
- if (repo_submodule_init(, the_repository, path))
+ if (repo_submodule_init(, the_repository, sub))
die(_("could not get a repository handle for submodule '%s'"),
path);
if (!repo_config_get_string(, "core.worktree", )) {
diff --git a/repository.c b/repository.c
index 5dd1486718..aabe64ee5d 100644
---
[PATCH 03/10] submodule.c: sort changed_submodule_names before searching it
We can string_list_insert() to maintain sorted-ness of the list as we find new items, or we can string_list_append() to build an unsorted list and sort it at the end just once. As we do not rely on the sortedness while building the list, we pick the "append and sort at the end" as it has better worst case execution times. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- submodule.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/submodule.c b/submodule.c index e145ebbb16..9fbfcfcfe1 100644 --- a/submodule.c +++ b/submodule.c @@ -1270,7 +1270,7 @@ static int get_next_submodule(struct child_process *cp, case RECURSE_SUBMODULES_DEFAULT: case RECURSE_SUBMODULES_ON_DEMAND: if (!submodule || - !unsorted_string_list_lookup( + !string_list_lookup( _submodule_names, submodule->name)) continue; @@ -1364,6 +1364,7 @@ int fetch_populated_submodules(struct repository *r, /* default value, "--submodule-prefix" and its value are added later */ calculate_changed_submodule_paths(); + string_list_sort(_submodule_names); run_processes_parallel(max_parallel_jobs, get_next_submodule, fetch_start_failure, -- 2.19.0
[PATCH 04/10] submodule.c: tighten scope of changed_submodule_names struct
The `changed_submodule_names` are only used for fetching, so let's make it
part of the struct that is passed around for fetching submodules.
Signed-off-by: Stefan Beller
---
submodule.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/submodule.c b/submodule.c
index 9fbfcfcfe1..6fb0b9d783 100644
--- a/submodule.c
+++ b/submodule.c
@@ -24,7 +24,6 @@
#include "object-store.h"
static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
-static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
static int initialized_fetch_ref_tips;
static struct oid_array ref_tips_before_fetch;
static struct oid_array ref_tips_after_fetch;
@@ -1124,7 +1123,8 @@ void check_for_new_submodule_commits(struct object_id
*oid)
oid_array_append(_tips_after_fetch, oid);
}
-static void calculate_changed_submodule_paths(void)
+static void calculate_changed_submodule_paths(
+ struct string_list *changed_submodule_names)
{
struct argv_array argv = ARGV_ARRAY_INIT;
struct string_list changed_submodules = STRING_LIST_INIT_DUP;
@@ -1162,7 +1162,8 @@ static void calculate_changed_submodule_paths(void)
continue;
if (!submodule_has_commits(path, commits))
- string_list_append(_submodule_names,
name->string);
+ string_list_append(changed_submodule_names,
+ name->string);
}
free_submodules_oids(_submodules);
@@ -1208,8 +1209,10 @@ struct submodule_parallel_fetch {
int default_option;
int quiet;
int result;
+
+ struct string_list changed_submodule_names;
};
-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0,
STRING_LIST_INIT_DUP }
static int get_fetch_recurse_config(const struct submodule *submodule,
struct submodule_parallel_fetch *spf)
@@ -1271,7 +1274,7 @@ static int get_next_submodule(struct child_process *cp,
case RECURSE_SUBMODULES_ON_DEMAND:
if (!submodule ||
!string_list_lookup(
- _submodule_names,
+ >changed_submodule_names,
submodule->name))
continue;
default_argv = "on-demand";
@@ -1363,8 +1366,8 @@ int fetch_populated_submodules(struct repository *r,
argv_array_push(, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
- calculate_changed_submodule_paths();
- string_list_sort(_submodule_names);
+ calculate_changed_submodule_paths(_submodule_names);
+ string_list_sort(_submodule_names);
run_processes_parallel(max_parallel_jobs,
get_next_submodule,
fetch_start_failure,
@@ -1373,7 +1376,7 @@ int fetch_populated_submodules(struct repository *r,
argv_array_clear();
out:
- string_list_clear(_submodule_names, 1);
+ string_list_clear(_submodule_names, 1);
return spf.result;
}
--
2.19.0
[PATCH 02/10] submodule.c: fix indentation
The submodule subsystem is really bad at staying within 80 characters.
Fix it while we are here.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
submodule.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/submodule.c b/submodule.c
index 2b7082b2db..e145ebbb16 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1258,7 +1258,8 @@ static int get_next_submodule(struct child_process *cp,
if (!submodule) {
const char *name = default_name_or_path(ce->name);
if (name) {
- default_submodule.path = default_submodule.name
= name;
+ default_submodule.path = name;
+ default_submodule.name = name;
submodule = _submodule;
}
}
@@ -1268,8 +1269,10 @@ static int get_next_submodule(struct child_process *cp,
default:
case RECURSE_SUBMODULES_DEFAULT:
case RECURSE_SUBMODULES_ON_DEMAND:
- if (!submodule ||
!unsorted_string_list_lookup(_submodule_names,
-submodule->name))
+ if (!submodule ||
+ !unsorted_string_list_lookup(
+ _submodule_names,
+ submodule->name))
continue;
default_argv = "on-demand";
break;
--
2.19.0
[PATCH 00/10] Resending sb/submodule-recursive-fetch-gets-the-tip
Thanks Jonathan for your thoughtful comments,
here is the product of the discussion:
* I split up the patch to fetch in the worktree to be 2 patches,
each giving motivation on its own.
* the last patch is vastly simplified in code, but takes an extra test
* in [1], you remark "commits" ought not to be a pointer, but I decided against
that, as we keep the pointed-at value around for the same time span (until
we're done with that submodule) and we don't need to copy over the pointed-at
value into the new struct.
[1]
https://public-inbox.org/git/20181018003954.139498-1-jonathanta...@google.com/
This is still based on ao/submodule-wo-gitmodules-checked-out.
previous version
https://public-inbox.org/git/20181016181327.107186-1-sbel...@google.com/
Stefan Beller (10):
sha1-array: provide oid_array_filter
submodule.c: fix indentation
submodule.c: sort changed_submodule_names before searching it
submodule.c: tighten scope of changed_submodule_names struct
submodule: store OIDs in changed_submodule_names
repository: repo_submodule_init to take a submodule struct
submodule: migrate get_next_submodule to use repository structs
submodule.c: fetch in submodules git directory instead of in worktree
fetch: try fetching submodules if needed objects were not fetched
builtin/fetch: check for submodule updates in any ref update
Documentation/technical/api-oid-array.txt| 5 +
builtin/fetch.c | 11 +-
builtin/grep.c | 17 +-
builtin/ls-files.c | 12 +-
builtin/submodule--helper.c | 2 +-
repository.c | 27 +-
repository.h | 12 +-
sha1-array.c | 17 ++
sha1-array.h | 3 +
submodule.c | 265 ---
t/helper/test-submodule-nested-repo-config.c | 8 +-
t/t5526-fetch-submodules.sh | 55
12 files changed, 346 insertions(+), 88 deletions(-)
git range-diff origin/sb/submodule-recursive-fetch-gets-the-tip...
1: 3fbb06aedd ! 1: 0fdb0e2ad9 submodule.c: move global
changed_submodule_names into fetch submodule struct
@@ -1,12 +1,11 @@
Author: Stefan Beller
-submodule.c: move global changed_submodule_names into fetch submodule
struct
+submodule.c: tighten scope of changed_submodule_names struct
The `changed_submodule_names` are only used for fetching, so let's
make it
part of the struct that is passed around for fetching submodules.
Signed-off-by: Stefan Beller
-Signed-off-by: Junio C Hamano
diff --git a/submodule.c b/submodule.c
--- a/submodule.c
@@ -16,7 +15,6 @@
static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
-static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
-+
static int initialized_fetch_ref_tips;
static struct oid_array ref_tips_before_fetch;
static struct oid_array ref_tips_after_fetch;
@@ -25,22 +23,8 @@
}
-static void calculate_changed_submodule_paths(void)
-+struct submodule_parallel_fetch {
-+ int count;
-+ struct argv_array args;
-+ struct repository *r;
-+ const char *prefix;
-+ int command_line_option;
-+ int default_option;
-+ int quiet;
-+ int result;
-+
-+ struct string_list changed_submodule_names;
-+};
-+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0,
STRING_LIST_INIT_DUP }
-+
+static void calculate_changed_submodule_paths(
-+ struct submodule_parallel_fetch *spf)
++ struct string_list *changed_submodule_names)
{
struct argv_array argv = ARGV_ARRAY_INIT;
struct string_list changed_submodules = STRING_LIST_INIT_DUP;
@@ -49,30 +33,23 @@
if (!submodule_has_commits(path, commits))
- string_list_append(_submodule_names,
name->string);
-+ string_list_append(>changed_submodule_names,
++ string_list_append(changed_submodule_names,
+ name->string);
}
free_submodules_oids(_submodules);
@@
- return ret;
- }
-
--struct submodule_parallel_fetch {
-- int count;
-- struct argv_array args;
-- struct repository *r;
-- const char *prefix;
-- int command_line_option;
-- int default_option;
-- int quiet;
-- int result;
--};
+ int default_option;
+ int quiet;
+ int result;
++
++ struct string_list changed_submodule_names;
+ };
-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
--
++#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0,
STRING_LIST_INIT_DUP }
+
static int
[PATCH 01/10] sha1-array: provide oid_array_filter
Helped-by: Junio C Hamano
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Documentation/technical/api-oid-array.txt | 5 +
sha1-array.c | 17 +
sha1-array.h | 3 +++
3 files changed, 25 insertions(+)
diff --git a/Documentation/technical/api-oid-array.txt
b/Documentation/technical/api-oid-array.txt
index 9febfb1d52..c97428c2c3 100644
--- a/Documentation/technical/api-oid-array.txt
+++ b/Documentation/technical/api-oid-array.txt
@@ -48,6 +48,11 @@ Functions
is not sorted, this function has the side effect of sorting
it.
+`oid_array_filter`::
+ Apply the callback function `want` to each entry in the array,
+ retaining only the entries for which the function returns true.
+ Preserve the order of the entries that are retained.
+
Examples
diff --git a/sha1-array.c b/sha1-array.c
index 265941fbf4..d505a004bb 100644
--- a/sha1-array.c
+++ b/sha1-array.c
@@ -77,3 +77,20 @@ int oid_array_for_each_unique(struct oid_array *array,
}
return 0;
}
+
+void oid_array_filter(struct oid_array *array,
+ for_each_oid_fn want,
+ void *cb_data)
+{
+ unsigned nr = array->nr, src, dst;
+ struct object_id *oids = array->oid;
+
+ for (src = dst = 0; src < nr; src++) {
+ if (want([src], cb_data)) {
+ if (src != dst)
+ oidcpy([dst], [src]);
+ dst++;
+ }
+ }
+ array->nr = dst;
+}
diff --git a/sha1-array.h b/sha1-array.h
index 232bf95017..55d016c4bf 100644
--- a/sha1-array.h
+++ b/sha1-array.h
@@ -22,5 +22,8 @@ int oid_array_for_each(struct oid_array *array,
int oid_array_for_each_unique(struct oid_array *array,
for_each_oid_fn fn,
void *data);
+void oid_array_filter(struct oid_array *array,
+ for_each_oid_fn want,
+ void *cbdata);
#endif /* SHA1_ARRAY_H */
--
2.19.0
Re: [PATCH] revision.c: drop missing objects from cmdline
On Tue, Oct 23, 2018 at 9:54 PM Junio C Hamano wrote: > > Matthew DeVore writes: > > > No code which reads cmdline in struct rev_info can handle NULL objects > > in cmdline.rev[i].item, so stop adding them to the cmdline.rev array. > > "The code is not prepared to have cmdline.rev[].item that is NULL" > is something everybody would understand and agree with, but that > does not automatically lead to "so ignoring or rejecting and dying > is OK", though. The cmdline thing is used for the commands to learn > the end-user intent that cannot be learned by the resulting objects > in the object array (e.g. the user may have said 'master' but the > pending[] (and later revs.commits) would only have the object names, > and some callers would want to know if it was a branch name, a > refname refs/heads/master, or the hexadecimal object name), so > unless absolutely needed, I'm hesitant to take a change that loses > information (e.g. the user named this object that is not locally > available, we cannot afford to add it to the pending[] and add it to > revs.commits to traverse from there, but we still want to know what > object was given by the user). Hmm, when you explain the purpose of cmdline, it's obvious now that it doesn't make sense to mechanically drop items from it. I'm sending another version of this patch which uses a more focused approach and is a bit simpler. > > > Objects in cmdline are NULL when the given object is promisor and > > --exclude-promisor-objects is enabled. > > A "promisor" is a remote repository. It promises certain objects > that you do not have are later retrievable from it. The way you can > see if the promisor promised to later give you an object is to see > if that missing object is reachable from an object in a packfile the > promisor gave you earlier. > > "The given object" is never a "promisor", so I am not sure what the > above wants to say. Is > > When an object is given on the command line and if it is missing > from the local repository, add_rev_cmdline() receives NULL in > its "item" parameter. > > what you meant? Is that the _only_ case in which "item" could be > NULL, or is it also true for any missing object due to repository > corruption? Yes, that is what I meant. I believe for corruption there is an actual error shown with die() or the like, though I am not certain.
Re: [PATCH] upload-pack: fix broken if/else chain in config callback
On 2018.10.24 10:50, Johannes Schindelin wrote: > Maybe a lot of explanation, but definitely a good one. The explanation and > the patch look good to me. > > Thanks, > Dscho Agreed, as a newbie I definitely appreciate detailed explanations. Looks good to me as well. Reviewed-by: Josh Steadmon
git filter-branch --filter-renames ?
All,
I recently needed to extract the git history of a portion of an existing
repository. My initial attempts using --subdirectory-filter, subtrees,
etc weren't as successful as I'd hoped.
The primary reason for my failures were due to the fact that this
particular source repository has seen a lot of code movement and renames
in-place. As a result, filters such as subdirectory filter failed to
keep commits prior to the renames.
So, long story short, I've attached below a hacked together script (yes,
it's sad when one writes a script to call a script :-/) that solves the
problem for me.
My hope is that some other poor sob in my position discovers this
script, uses it and moves on. If enough people think it's useful
despite the cornercases [1], I'd be happy to work on integrating it into
filter-branch.
thx,
Jason.
[1] Namely that if two different files held the same full-path name at
different times in the source repo, you'll get some errant commits in
the history.
--->8--
#!/bin/bash
#
# git-filter-renames: Similar to --subdirectory-filter but tracks renames
#
# Basic use:
# $ git clone path/to/source_repo dest_repo
# $ cd dest_repo
# $ git tags | xargs git tag -d # ours are signed, so would fail to verify
# $ git remote remove origin
# $ git gc --aggressive --prune=now --force
# $ git fsck
# $ git-filter-renames.sh "[PREFIX] " fileA subdirB/ fileC subdirD/subdirE ...
# $ rm -rf .git/refs/original
# $ git gc --aggressive --prune=now --force
# $ git fsck
DEBUG=1
if [ $# -le 1 ]; then
echo >&2 "Usage:"
echo >&2 "${0##*/} '[subj prefix] ' fileA fileB dir1 sub/dir2"
echo >&2 ""
exit 1
fi
if [ $DEBUG == 1 ]; then
rm -rf /tmp/git-filter-renames-*
fi
TMP_DIR="`mktemp -d /tmp/git-filter-renames-XX`"
PREFIX="${1}"
shift
# take in the list of files to preserve
# note: directories are recursed
echo -n "" >$TMP_DIR/user_list.txt
for arg in $*; do
if [ -d "$arg" ]; then
find $arg -type f >>$TMP_DIR/user_list.txt
elif [ -f "$arg" ]; then
echo "$arg" >>$TMP_DIR/user_list.txt
else
echo >&2 "What the hell is '$arg'?"
fi
done
echo -n "" >$TMP_DIR/trace_list.txt
while read fn <&4; do
while read ofn <&5; do
echo "^$ofn\$"
done 5< <(git log --format=format: --follow --name-only -- "$fn" | \
sed -e '/^$/d' | sort -u)
done 4< <(cat $TMP_DIR/user_list.txt) | sort -u >>$TMP_DIR/trace_list.txt
# stage the filter script
cat >$TMP_DIR/filter.sh <$TMP_DIR/msg_filter.sh <&2 "Doing filtering"
git filter-branch --prune-empty -f --index-filter "$TMP_DIR/filter.sh" \
--msg-filter "$TMP_DIR/msg_filter.sh" \
HEAD
# cleanup
if [ $DEBUG == 0 ]; then
rm -rf $TMP_DIR
fi
Re: [PATCH 8/9] fetch: retry fetching submodules if needed objects were not fetched
On Tue, Oct 23, 2018 at 4:38 PM Jonathan Tan wrote: > > > > Another thing you need to clarify is what happens if the fetch-by-commit > > > fails. Right now, it seems that it will make the whole thing fail, which > > > might be a surprising change in behavior. > > > > But a positive surprise, I would assume? > > Whether positive or negative, I think that this needs to be mentioned in > the commit message. > > As for positive or negative, I tend to agree that it's positive - sure, > some previously successful fetches would now fail, but the results of > those fetches could not be recursively checked out anyway. > > > > The test stores the result in a normal branch, not a remote tracking > > > branch. Is storing in a normal branch required? > > > > In the test we fetch from another repository, such that in the > > repository-under-test this will show up in a remote tracking branch? I messed up there. Yes, we need to fetch into a normal branch such that the logic of check_for_new_submodule_commits triggers no matter where it is on the remote. Your experiment below shows that we cannot fetch into FETCH_HEAD: > If that were true, I would expect that when this line: > > > git fetch --recurse-submodules --recurse-submodules-default on-demand > > origin refs/changes/2:refs/heads/my_branch && > > is replaced by this line: > > > git fetch --recurse-submodules --recurse-submodules-default on-demand > > origin refs/changes/2 && > > then things would still work. The tests pass with the first line (after > I fixed a type mismatch) but not with the second. (Also I don't think a > remote-tracking branch is generated here - the output printed doesn't > indicate so, and refs/changes/2 is not a branch anyway.) > > > Also, do you know why this is required? A naive reading of the patch > > > leads me to believe that this should work even if merely fetching to > > > FETCH_HEAD. > > > > See the next patch, check_for_new_submodule_commits() is missing > > for FETCH_HEAD. > > I see in the next patch that there is an "if" branch in > store_updated_refs() without update_local_ref() in which > "check_for_new_submodule_commits(>old_oid)" needs to be inserted. I > think this is a symptom that maybe check_for_new_submodule_commits() > needs to be extracted from update_local_ref() and put into > store_updated_refs() instead? In update_local_ref(), it is called on > ref->new_oid, which is actually the same as rm->old_oid anyway (there is > an oidcpy earlier). I'll look into that. > > > What is a "default" submodule and why would you need one? > > > > s/default/artificial/. Such a submodule is a submodule that has no > > config in the .gitmodules file and its name == path. > > We need to keep those around for historic reasons AFAICT, c.f. > > c68f837576 (implement fetching of moved submodules, 2017-10-16) > > Ah, OK. I would call it a fake submodule then, and copy over the "No > entry in .gitmodules?" comment. "fake submodule" sounds like http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb which is what I think of when hearing fake submodules.
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On Thu, Oct 25, 2018 at 02:24:41AM +0100, Ramsay Jones wrote: > >> Yeah, my thinko. The latter would be closer to what this patch > >> wants to have, but obviously the former would be more flexible and > >> useful in wider context. Both have the "Huh?" factor---what they > >> are doing has little to do with "config", but I did not think of a > >> better kitchen-sink (and our default kitchen-sink "rev-parse" is > >> even further than "config", I would think, for this one). > > > > Heh, I thought through the exact sequence in your paragraph when writing > > my other message. That's probably a good sign that we should probably > > not pursue this further unless we see the use case come up again a few > > more times (and if we do, then consider "config" the least-bad place to > > do it). > > I was thinking: > > $ git var -e GIT_WHATEVER_ENV > > [-e for environment]. > > ... but that is really no different than git-config. ;-) Actually, "git var" already does pull bits from the environment. It doesn't know about all of the type-specific parsing that git-config does, but it might be a reasonable path forward to teach it that. (But I still think we should do nothing for now and see how often this comes up). -Peff
Re: 'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress'
On Thu, Oct 25 2018, SZEDER Gábor wrote: > when branch 'ds/test-multi-pack-index' is merged with > 'ab/commit-graph-progress', IOW 'master', 'next', or 'pu', > 'GIT_TEST_MULTI_PACK_INDEX=1 ./t6500-gc.sh' fails with: > > expecting success: > git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && > test_must_be_empty stdout && > test_line_count = 1 stderr && > test_i18ngrep "Computing commit graph generation numbers" stderr > > + git -c gc.writeCommitGraph=true gc --no-quiet > + test_must_be_empty stdout > + test_path_is_file stdout > + test -f stdout > + test -s stdout > + test_line_count = 1 stderr > + test 3 != 3 > + wc -l > + test 16 = 1 > + echo test_line_count: line count for stderr != 1 > test_line_count: line count for stderr != 1 > + cat stderr > error: packfile > .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index > unavailable > error: packfile > .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index > unavailable > error: packfile > .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index > unavailable > error: packfile > .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index > unavailable > error: packfile > .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index > unavailable > error: packfile > .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index > unavailable > error: packfile > .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index > unavailable > error: packfile > .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index > unavailable > error: packfile > .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index > unavailable > error: packfile > .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index > unavailable > error: packfile > .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index > unavailable > error: packfile > .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index > unavailable > error: packfile > .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index > unavailable > error: packfile > .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index > unavailable > error: packfile > .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index > unavailable > Computing commit graph generation numbers: 25% (1/4) ^MComputing commit > graph generation numbers: 50% (2/4) ^MComputing commit graph generation > numbers: 75% (3/4) ^MComputing commit graph generation numbers: 100% (4/4) > ^MComputing commit graph generation numbers: 100% (4/4), done. > + return 1 > error: last command exited with $?=1 > not ok 9 - gc --no-quiet > > > I suspect these "packfile index unavailable" errors are a Bad Thing, > but I didn't follow the MIDX development closely enough to judge. > Surprisingly (to me), 'git gc' didn't exit with error despite these > errors. > > Anyway, this test seems to be too fragile, because that > > test_line_count = 1 stderr Yeah maybe it's too fragile, on the other hand it caught what seems to be a bug under GIT_TEST_MULTI_PACK_INDEX=true, and the rest of the test suite passes, so there's that. > line will trigger, when anything else during 'git gc' prints > something. And I find it quite strange that an option called > '--no-quiet' only shows the commit-graph progress, but not the regular > output of 'git gc'. It's confusing, but the reason for this seeming discrepancy is that writing the commit-graph happens in-process, whereas the rest of the work done by git-gc (and its output) comes from subprocesses. Most of that output is from "git-repack" / "git-pack-objects" which doesn't pay the same attention to --quiet and --no-quiet, instead it checks isatty(). See cmd_pack_objects().
Re: [PATCH v5] archive: initialize archivers earlier
On Thu, Oct 25, 2018 at 01:32:14PM -0700, stead...@google.com wrote:
> Initialize archivers as soon as possible when running git-archive.
> Various non-obvious behavior depends on having the archivers
> initialized, such as determining the desired archival format from the
> provided filename.
>
> Since 08716b3c11 ("archive: refactor file extension format-guessing",
> 2011-06-21), archive_format_from_filename() has used the registered
> archivers to match filenames (provided via --output) to archival
> formats. However, when git-archive is executed with --remote, format
> detection happens before the archivers have been registered. This causes
> archives from remotes to always be generated as TAR files, regardless of
> the actual filename (unless an explicit --format is provided).
>
> This patch fixes that behavior; archival format is determined properly
> from the output filename, even when --remote is used.
Thanks, this version looks great to me!
-Peff
Re: [PATCH v3 3/3] commit-slab: missing definitions and forward declarations (hdr-check)
On 25/10/2018 19:54, Ramsay Jones wrote: > > > On 25/10/2018 12:04, Carlo Marcelo Arenas Belón wrote: >> struct commmit needs to be defined before commit-slab can generate >> working code, object_id should be at least known through a forward >> declaration >> >> Signed-off-by: Carlo Marcelo Arenas Belón >> --- >> commit-slab-impl.h | 2 ++ >> commit-slab.h | 2 ++ >> 2 files changed, 4 insertions(+) >> >> diff --git a/commit-slab-impl.h b/commit-slab-impl.h >> index e352c2f8c1..db7cf3f19b 100644 >> --- a/commit-slab-impl.h >> +++ b/commit-slab-impl.h >> @@ -1,6 +1,8 @@ >> #ifndef COMMIT_SLAB_IMPL_H >> #define COMMIT_SLAB_IMPL_H >> >> +#include "commit.h" >> + >> #define implement_static_commit_slab(slabname, elemtype) \ >> implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static) >> >> diff --git a/commit-slab.h b/commit-slab.h >> index 69bf0c807c..722252de61 100644 >> --- a/commit-slab.h >> +++ b/commit-slab.h >> @@ -1,6 +1,8 @@ >> #ifndef COMMIT_SLAB_H >> #define COMMIT_SLAB_H >> >> +struct object_id; >> + >> #include "commit-slab-decl.h" >> #include "commit-slab-impl.h" >> >> > > Hmm, sorry, I don't see how this patch has anything to do > with the other two patches! ;-) > > Also, I have a patch to fix up the 'commit-reach.h' header > (it was part of my original series, just had to update the > commit message), which adds these very #includes and forward > declarations when _using_ the commit-slab. > > I haven't tried applying your patches yet, which may answer > my questions, so I am a little puzzled. So, having now applied your patches, I still don't see what this patch has to do with the others! I suppose it is dependent on the compiler/version? (the most up-to-date version of clang available to me is 5.0.1). Yes, this will 'fix' the 'commit-reach.h' header (not surprising), but I prefer my patch. ;-) Still puzzled. ATB, Ramsay Jones
Re: [PATCH sg/test-rebase-editor-fix] t3404-rebase-interactive: test abbreviated commands
Am 25.10.18 um 22:54 schrieb Johannes Sixt: Test each short command at least once. The commands changed here are chosen such that - tests do not have a prerequisite, - the 'git rebase' command is not guarded by test_must_fail. The pick commands are optional noise words in the FAKE_LINES variable. Test them, too. Actually, this sentence should better be: The pick commands are optional in the FAKE_LINES variable, but when used, they do end up in the insn sheet. Test them, too. Signed-off-by: Johannes Sixt ... @@ -732,7 +732,7 @@ test_expect_success 'reword' ' git show HEAD^ | grep "D changed" && FAKE_LINES="reword 1 2 3 4" FAKE_COMMIT_MESSAGE="B changed" git rebase -i A && git show HEAD~3 | grep "B changed" && - FAKE_LINES="1 reword 2 3 4" FAKE_COMMIT_MESSAGE="C changed" git rebase -i A && + FAKE_LINES="1 r 2 pick 3 p 4" FAKE_COMMIT_MESSAGE="C changed" git rebase -i A && git show HEAD~2 | grep "C changed" '
[PATCH sg/test-rebase-editor-fix] t3404-rebase-interactive: test abbreviated commands
Test each short command at least once. The commands changed here
are chosen such that
- tests do not have a prerequisite,
- the 'git rebase' command is not guarded by test_must_fail.
The pick commands are optional noise words in the FAKE_LINES
variable. Test them, too.
Signed-off-by: Johannes Sixt
---
This patch must be placed on top of sg/test-rebase-editor-fix.
It has a textual conflict with my sequencer 'b' fix from some
minutes ago, but the resolution should be obvious:
- exec*|break|b)
- exec_*|x_*)
++ exec_*|x_*|break|b)
t/lib-rebase.sh | 4 ++--
t/t3404-rebase-interactive.sh | 10 +-
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 2ca9fb69d6..0c93d00bdd 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -47,9 +47,9 @@ set_fake_editor () {
action=pick
for line in $FAKE_LINES; do
case $line in
- pick|squash|fixup|edit|reword|drop)
+ pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
action="$line";;
- exec*)
+ exec_*|x_*)
echo "$line" | sed 's/_/ /g' >> "$1";;
"#")
echo '# comment' >> "$1";;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 352a52e59d..d36ee4f807 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -114,7 +114,7 @@ test_expect_success 'rebase -i with exec allows git
commands in subdirs' '
git checkout master &&
mkdir subdir && (cd subdir &&
set_fake_editor &&
- FAKE_LINES="1 exec_cd_subdir_&&_git_rev-parse_--is-inside-work-tree" \
+ FAKE_LINES="1 x_cd_subdir_&&_git_rev-parse_--is-inside-work-tree" \
git rebase -i HEAD^
)
'
@@ -499,7 +499,7 @@ test_expect_success 'squash works as expected' '
git checkout -b squash-works no-conflict-branch &&
one=$(git rev-parse HEAD~3) &&
set_fake_editor &&
- FAKE_LINES="1 squash 3 2" EXPECT_HEADER_COUNT=2 \
+ FAKE_LINES="1 s 3 2" EXPECT_HEADER_COUNT=2 \
git rebase -i HEAD~3 &&
test $one = $(git rev-parse HEAD~2)
'
@@ -732,7 +732,7 @@ test_expect_success 'reword' '
git show HEAD^ | grep "D changed" &&
FAKE_LINES="reword 1 2 3 4" FAKE_COMMIT_MESSAGE="B changed" git rebase
-i A &&
git show HEAD~3 | grep "B changed" &&
- FAKE_LINES="1 reword 2 3 4" FAKE_COMMIT_MESSAGE="C changed" git rebase
-i A &&
+ FAKE_LINES="1 r 2 pick 3 p 4" FAKE_COMMIT_MESSAGE="C changed" git
rebase -i A &&
git show HEAD~2 | grep "C changed"
'
@@ -758,7 +758,7 @@ test_expect_success 'rebase -i can copy notes over a fixup'
'
git reset --hard n3 &&
git notes add -m"an earlier note" n2 &&
set_fake_editor &&
- GIT_NOTES_REWRITE_MODE=concatenate FAKE_LINES="1 fixup 2" git rebase -i
n1 &&
+ GIT_NOTES_REWRITE_MODE=concatenate FAKE_LINES="1 f 2" git rebase -i n1
&&
git notes show > output &&
test_cmp expect output
'
@@ -1208,7 +1208,7 @@ rebase_setup_and_clean () {
test_expect_success 'drop' '
rebase_setup_and_clean drop-test &&
set_fake_editor &&
- FAKE_LINES="1 drop 2 3 drop 4 5" git rebase -i --root &&
+ FAKE_LINES="1 drop 2 3 d 4 5" git rebase -i --root &&
test E = $(git cat-file commit HEAD | sed -ne \$p) &&
test C = $(git cat-file commit HEAD^ | sed -ne \$p) &&
test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
--
2.19.1.406.g1aa3f475f3
[PATCH 3/2] rebase -i: recognize short commands without arguments
The sequencer instruction 'b', short for 'break', is rejected:
error: invalid line 2: b
The reason is that the parser expects all short commands to have
an argument. Permit short commands without arguments.
Signed-off-by: Johannes Sixt
---
I'll send a another patch in a moment that tests all short
sequencer commands, but it is independent from this topic.
sequencer.c| 3 ++-
t/lib-rebase.sh| 2 +-
t/t3418-rebase-continue.sh | 4 +++-
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index ee3961ec63..3107f59ea7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1954,7 +1954,8 @@ static int parse_insn_line(struct todo_item *item, const
char *bol, char *eol)
if (skip_prefix(bol, todo_command_info[i].str, )) {
item->command = i;
break;
- } else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
+ } else if ((bol + 1 == eol || bol[1] == ' ') &&
+ *bol == todo_command_info[i].c) {
bol++;
item->command = i;
break;
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 584604ee63..86572438ec 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -49,7 +49,7 @@ set_fake_editor () {
case $line in
squash|fixup|edit|reword|drop)
action="$line";;
- exec*|break)
+ exec*|break|b)
echo "$line" | sed 's/_/ /g' >> "$1";;
"#")
echo '# comment' >> "$1";;
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 185a491089..b282505aac 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -243,7 +243,9 @@ unset GIT_SEQUENCE_EDITOR
test_expect_success 'the todo command "break" works' '
rm -f execed &&
- FAKE_LINES="break exec_>execed" git rebase -i HEAD &&
+ FAKE_LINES="break b exec_>execed" git rebase -i HEAD &&
+ test_path_is_missing execed &&
+ git rebase --continue &&
test_path_is_missing execed &&
git rebase --continue &&
test_path_is_file execed
--
2.19.1.406.g1aa3f475f3
[PATCH v5] archive: initialize archivers earlier
Initialize archivers as soon as possible when running git-archive.
Various non-obvious behavior depends on having the archivers
initialized, such as determining the desired archival format from the
provided filename.
Since 08716b3c11 ("archive: refactor file extension format-guessing",
2011-06-21), archive_format_from_filename() has used the registered
archivers to match filenames (provided via --output) to archival
formats. However, when git-archive is executed with --remote, format
detection happens before the archivers have been registered. This causes
archives from remotes to always be generated as TAR files, regardless of
the actual filename (unless an explicit --format is provided).
This patch fixes that behavior; archival format is determined properly
from the output filename, even when --remote is used.
Signed-off-by: Josh Steadmon
Helped-by: Jeff King
---
Range-diff against v4:
1: 1d7b070928 ! 1: c85673cee7 archive: initialize archivers earlier
@@ -96,7 +96,7 @@
+test_expect_success GZIP 'git archive with --output and --remote creates
.tgz' '
+ git archive --output=d5.tgz --remote=. HEAD &&
-+ gzip -d -c < d5.tgz > d5.tar &&
++ gzip -d -c d5.tar &&
+ test_cmp_bin b.tar d5.tar
+'
+
archive.c| 9 ++---
archive.h| 1 +
builtin/archive.c| 2 ++
builtin/upload-archive.c | 2 ++
t/t5000-tar-tree.sh | 6 ++
t/t5003-archive-zip.sh | 7 ++-
6 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/archive.c b/archive.c
index c1870105eb..ce0f8a0362 100644
--- a/archive.c
+++ b/archive.c
@@ -29,6 +29,12 @@ void register_archiver(struct archiver *ar)
archivers[nr_archivers++] = ar;
}
+void init_archivers(void)
+{
+ init_tar_archiver();
+ init_zip_archiver();
+}
+
static void format_subst(const struct commit *commit,
const char *src, size_t len,
struct strbuf *buf)
@@ -531,9 +537,6 @@ int write_archive(int argc, const char **argv, const char
*prefix,
git_config_get_bool("uploadarchive.allowunreachable",
_allow_unreachable);
git_config(git_default_config, NULL);
- init_tar_archiver();
- init_zip_archiver();
-
args.repo = repo;
argc = parse_archive_args(argc, argv, , , name_hint, remote);
if (!startup_info->have_repository) {
diff --git a/archive.h b/archive.h
index d4f97a00f5..21ac010699 100644
--- a/archive.h
+++ b/archive.h
@@ -43,6 +43,7 @@ extern void register_archiver(struct archiver *);
extern void init_tar_archiver(void);
extern void init_zip_archiver(void);
+extern void init_archivers(void);
typedef int (*write_archive_entry_fn_t)(struct archiver_args *args,
const struct object_id *oid,
diff --git a/builtin/archive.c b/builtin/archive.c
index e74f675390..d2455237ce 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -97,6 +97,8 @@ int cmd_archive(int argc, const char **argv, const char
*prefix)
argc = parse_options(argc, argv, prefix, local_opts, NULL,
PARSE_OPT_KEEP_ALL);
+ init_archivers();
+
if (output)
create_output_file(output);
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 25d9116356..018879737a 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -28,6 +28,8 @@ int cmd_upload_archive_writer(int argc, const char **argv,
const char *prefix)
if (!enter_repo(argv[1], 0))
die("'%s' does not appear to be a git repository", argv[1]);
+ init_archivers();
+
/* put received options in sent_argv[] */
argv_array_push(_argv, "git-upload-archive");
for (;;) {
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 2a97b27b0a..602bfd9574 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -206,6 +206,12 @@ test_expect_success 'git archive with --output, override
inferred format' '
test_cmp_bin b.tar d4.zip
'
+test_expect_success GZIP 'git archive with --output and --remote creates .tgz'
'
+ git archive --output=d5.tgz --remote=. HEAD &&
+ gzip -d -c d5.tar &&
+ test_cmp_bin b.tar d5.tar
+'
+
test_expect_success 'git archive --list outside of a git repo' '
nongit git archive --list
'
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 55c7870997..106eddbd85 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -158,11 +158,16 @@ test_expect_success 'git archive --format=zip with
--output' \
'git archive --format=zip --output=d2.zip HEAD &&
test_cmp_bin d.zip d2.zip'
-test_expect_success 'git archive with --output, inferring format' '
+test_expect_success 'git archive with --output, inferring format (local)' '
git archive --output=d3.zip HEAD &&
test_cmp_bin d.zip d3.zip
'
+test_expect_success 'git archive with --output,
Re: [PATCH v3] archive: initialize archivers earlier
On 2018.10.23 13:09, Junio C Hamano wrote: > stead...@google.com writes: > > > diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh > > index 2a97b27b0a..cfd5ca492f 100755 > > --- a/t/t5000-tar-tree.sh > > +++ b/t/t5000-tar-tree.sh > > @@ -39,6 +39,8 @@ test_lazy_prereq TAR_NEEDS_PAX_FALLBACK ' > > > > test_lazy_prereq GZIP 'gzip --version' > > > > +test_lazy_prereq ZIP 'zip --version' > > + > > There are a handful of zip implementations; Info-ZIP found on many > Linux distros does support 'zip --version', but we may want to make > sure this test covers different implementations of zip sufficiently. > > Queuing this patch (or an update of it) on 'pu' and hoping those > with zip from different origins to try it would not help very much, > either, as zip implementations that do not react to "zip --version" > would silently turn the prereq off without breaking anything. > > In any case, please refrain from adding any ZIP prerequiste to t5000 > which is about tar; t5003-archive-zip may be a much better fit. It > has an already working machinery that validates the generated zip > archive under UNZIP prerequisite, so we may not even have to invent > our own ZIP prereq if we did so. Ack. This has been removed in v4. V4 also has a test case in t5003 based on Jeff's advice. > > @@ -206,6 +208,19 @@ test_expect_success 'git archive with --output, > > override inferred format' ' > > test_cmp_bin b.tar d4.zip > > ' > > > > +test_expect_success GZIP 'git archive with --output and --remote creates > > .tgz' ' > > + git archive --output=d5.tgz --remote=. HEAD && > > + gzip -d -c < d5.tgz > d5.tar && > > + test_cmp_bin b.tar d5.tar > > +' > > We try to write redirections without SP between redirection operator > and target filename, i.e. "gzip -d -c d5.tar". Fixed in v5.
Re: [PATCH v5] branch: introduce --show-current display option
On Thu, Oct 25, 2018 at 3:04 PM Daniels Umanovskis wrote: > When called with --show-current, git branch will print the current > branch name and terminate. Only the actual name gets printed, > without refs/heads. In detached HEAD state, nothing is output. > > Signed-off-by: Daniels Umanovskis > --- > diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh > @@ -100,6 +100,50 @@ test_expect_success 'git branch -v pattern does not show > branch summaries' ' > +test_expect_success 'git branch `--show-current` works properly when tag > exists' ' > + cat >expect <<-\EOF && > + branch-and-tag-name > + EOF > + test_when_finished " > + git checkout branch-one > + git branch -D branch-and-tag-name > + " && > + git checkout -b branch-and-tag-name && > + test_when_finished "git tag -d branch-and-tag-name" && > + git tag branch-and-tag-name && If git-tag crashes before actually creating the new tag, then "git tag -d", passed to test_when_finished(), will error out too, which is probably undesirable since "cleanup code" isn't expected to error out. You could fix it this way: test_when_finished "git tag -d branch-and-tag-name || :" && git tag branch-and-tag-name && or, even better, just swap the two lines: git tag branch-and-tag-name && test_when_finished "git tag -d branch-and-tag-name" && However, do you even need to clean up the tag? Are there tests following this one which expect a certain set of tags and fail if this new one is present? If not, a simpler approach might be just to leave the tag alone (and the branch too if that doesn't need to be cleaned up). > + git branch --show-current >actual && > + test_cmp expect actual > +'
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
On Wed, Oct 24, 2018 at 6:59 PM SZEDER Gábor wrote: > > On Mon, Oct 22, 2018 at 11:54:06AM -0700, Stefan Beller wrote: > > > For the sake of a good history, I would think running 'make coccicheck' > > and applying the resulting patches would be best as part of the (dirty) > > merge of any topic that proposes new semantic patches, but that would > > add load to Junio as it would be an extra step during the merge. > > > > One could argue that the step of applying such transformations into > > the dirty merge is cheaper than resolving merge conflicts that are > > had when the topic includes the transformation. > > Please consider that merge commits' have uglier diffs than regular > commits, and that merge commits cause additional complications when > 'git bisect' points the finger at them, both of which are exacerbated > by additional changes squeezed into evil merges. > > > > Consequently, 'make coccicheck' won't run clean and the > > > static analysis build job will fail until all those topics reach > > > 'master', and the remaining transformations are applied on top. > > > > > > This was (and still is!) an issue with the hasheq()/oideq() series > > > as well: that series was added on 2018-08-28, and the static > > > analysis build job is red on 'pu' ever since. See the follow-up > > > patch e43d2dcce1 (more oideq/hasheq conversions, 2018-10-02), and > > > one more follow-up will be necessary after the builtin stash topic > > > is merged to 'master'. > > > > In my understanding this follow up is a feature, as it helps to avoid > > merge conflicts with other topics in flight. > > I don't see how such a follow up patch helps to avoid merge conflicts. Well, you merge first (the new topic and the cocci patches), and then do the transformation. But that is putting a lot more work on Junio as the way to integrate is not just merge a new topic into the pile of topics (whether it is pu/next/master), but to first perform a merge of the topic and the coccinelle patches, apply the transformation and then merge to the pile, assuming the pile is already transformed (as it happened in "treewide: apply cocci patch" in next/pu). > > as 'make coccicheck' is an integral part of your review? > > Erm, right, "review" was not the right word here. Anyway, as it is, > 'make coccicheck' is an integral part of our automated tests, not only > on Travis CI but on the upcoming Azure thing as well. I just try to > pay attention to its results and the results of a bunch of my > additional builds, and complain or even send a fix when something goes > reproducibly wrong. This has certainly became more cumbersome with > the permanently failing static analysis build job in the last couple > of weeks. I seem to not pay as much attention as I should to these, mostly because they are unreliable on the aggregate level (a failure there most likely means another topic than the one I am interested broke; except in this case, where we discuss the fallout there via this topic.) > > I like the approach of having separate classes of semantic patches: > > (a) the regular "we need to keep checking these" as they address > > undesirable code patterns, which is what we currently have, > > and what 'make coccicheck' would complain about. > > (b) The pending patches as you propose. However I would > > argue that we'd not want to include the transformation into > > the same patch as then the patch will have merge conflicts. > > Since we have a lot of parallel running topics, merge conflicts are > basically unavoidable anyway. If the conflicts from the > transformation are really that severe, then perhaps the whole series > should be postponed to a calmer, more suitable time. Merge conflicts of this kind could be avoided, by running the transformation on both sides before merging (or not running them on both sides, but only after merging). So maybe for these larger 'the_repository.pending.cocci' patches we could get them into the tree and wait until all (most) of the topics are including that semantic patch in their base, such that application of the patch is easy whether before or after writing a series (as the semantic patch is in its base). Another short term plan would be renaming the_repository.cocci such that it would break the 'make coccicheck' > In the case of 'the_repository.cocci', merging its transformations > into 'pu' resulted in only four conflicts, and I found all four on the > easy side to resolve. I don't think it's worth waiting with the > transformations in this particular case. I am not worried about the current conflicts, but about those to come in new series. > > > Ideally we'd have an automated process/bot that would apply > > all pending semantic patches onto master and then checks for > > conflicts in HEAD..pu, and only sends off the non-conflicting > > diffs as a topic. > > New semantic patches didn't pop up all that frequently in the past, so >
[PATCH v3 1/3] [Outreachy] t3903-stash: test without configured user name
From: Slavica This is part of enhancement request that ask for 'git stash' to work even if 'user.name' and 'user.email' are not configured. Due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't. The issue is discussed here: https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u. Signed-off-by: Slavica Djukic --- t/t3903-stash.sh | 14 ++ 1 file changed, 14 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 9e06494ba0..ae2c905343 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1156,4 +1156,18 @@ test_expect_success 'stash -- works with binary files' ' test_path_is_file subdir/untracked ' +test_expect_failure 'stash works when user.name and user.email are not set' ' +test_commit 1 && +test_config user.useconfigonly true && +test_config stash.usebuiltin true && +sane_unset GIT_AUTHOR_NAME && +sane_unset GIT_AUTHOR_EMAIL && +sane_unset GIT_COMMITTER_NAME && +sane_unset GIT_COMMITTER_EMAIL && +test_unconfig user.email && +test_unconfig user.name && +echo changed >1.t && +git stash +' + test_done -- 2.19.1.windows.1
[PATCH v3 1/3] [Outreachy] t3903-stash: test without configured user name
Changes since v1: *changed: test_must_fail git config user.email to: test_unconfig user.email && test_unconfig user.name This is done to make sure that user.email and user.name are not set, instead of asserting it with test_must_fail config user.email. Slavica (1): [Outreachy] t3903-stash: test without configured user name t/t3903-stash.sh | 14 ++ 1 file changed, 14 insertions(+) -- 2.19.1.windows.1
Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files
On Thu, Oct 25, 2018 at 1:47 AM Nickolai Belakovski wrote: > The motivation for the change is some work that I'm doing to add a > worktree atom in ref-filter.c. I wanted that atom to be able to access > all fields of the worktree struct and noticed that lock_reason wasn't > getting populated so I figured I'd go and fix that. > > Reviewing this work in the context of your feedback and Junio's > previous comments, I think it makes sense to only have a field in the > struct indicating whether or not the worktree is locked, and have a > separate function for getting the reason. Is your new ref-filter atom going to be boolean-only or will it also have a form (or a separate atom) for retrieving the lock-reason? I imagine both could be desirable. In any event, implementation-wise, I would think that such an atom (or atoms) could be easily built with the existing worktree API (with its lazy-loading and caching), which might be an easy way forward since you wouldn't need this patch or the updated one you posted[1], thus no need to justify such a change. > Since the only cases in > which the reason is retrieved in the current codebase are cases where > the program immediately dies, caching seems a moot point. If your new atom has a form for retrieving the lock reason, then caching could potentially be beneficial(?). [1]: https://public-inbox.org/git/20181025055142.38077-1-nbelakov...@gmail.com/
[PATCH v5] branch: introduce --show-current display option
When called with --show-current, git branch will print the current
branch name and terminate. Only the actual name gets printed,
without refs/heads. In detached HEAD state, nothing is output.
Intended both for scripting and interactive/informative use.
Unlike git branch --list, no filtering is needed to just get the
branch name.
Signed-off-by: Daniels Umanovskis
---
Submitting v5 now that a week has passed since latest maintainer
comments.
This is basically v4 but with small fixes to the test, as proposed
by Junio on pu, and additionally replacing a subshell
with { .. } since Dscho and Eric discovered the negative
performance effects of subshell invocations.
Documentation/git-branch.txt | 6 -
builtin/branch.c | 25 ++--
t/t3203-branch-output.sh | 44
3 files changed, 72 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index bf5316ffa9..0babb9b1be 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -9,7 +9,7 @@ SYNOPSIS
[verse]
'git branch' [--color[=] | --no-color] [-r | -a]
- [--list] [-v [--abbrev= | --no-abbrev]]
+ [--list] [--show-current] [-v [--abbrev= | --no-abbrev]]
[--column[=] | --no-column] [--sort=]
[(--merged | --no-merged) []]
[--contains []]
@@ -160,6 +160,10 @@ This option is only applicable in non-verbose mode.
branch --list 'maint-*'`, list only the branches that match
the pattern(s).
+--show-current::
+ Print the name of the current branch. In detached HEAD state,
+ nothing is printed.
+
-v::
-vv::
--verbose::
diff --git a/builtin/branch.c b/builtin/branch.c
index c396c41533..46f91dc06d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -443,6 +443,21 @@ static void print_ref_list(struct ref_filter *filter,
struct ref_sorting *sortin
free(to_free);
}
+static void print_current_branch_name(void)
+{
+ int flags;
+ const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, );
+ const char *shortname;
+ if (!refname)
+ die(_("could not resolve HEAD"));
+ else if (!(flags & REF_ISSYMREF))
+ return;
+ else if (skip_prefix(refname, "refs/heads/", ))
+ puts(shortname);
+ else
+ die(_("HEAD (%s) points outside of refs/heads/"), refname);
+}
+
static void reject_rebase_or_bisect_branch(const char *target)
{
struct worktree **worktrees = get_worktrees(0);
@@ -581,6 +596,7 @@ static int edit_branch_description(const char *branch_name)
int cmd_branch(int argc, const char **argv, const char *prefix)
{
int delete = 0, rename = 0, copy = 0, force = 0, list = 0;
+ int show_current = 0;
int reflog = 0, edit_description = 0;
int quiet = 0, unset_upstream = 0;
const char *new_upstream = NULL;
@@ -620,6 +636,7 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
OPT_BIT('c', "copy", , N_("copy a branch and its reflog"),
1),
OPT_BIT('C', NULL, , N_("copy a branch, even if target
exists"), 2),
OPT_BOOL('l', "list", , N_("list branch names")),
+ OPT_BOOL(0, "show-current", _current, N_("show current
branch name")),
OPT_BOOL(0, "create-reflog", , N_("create the branch's
reflog")),
OPT_BOOL(0, "edit-description", _description,
N_("edit the description for the branch")),
@@ -662,14 +679,15 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
0);
- if (!delete && !rename && !copy && !edit_description && !new_upstream
&& !unset_upstream && argc == 0)
+ if (!delete && !rename && !copy && !edit_description && !new_upstream &&
+ !show_current && !unset_upstream && argc == 0)
list = 1;
if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE ||
filter.points_at.nr ||
filter.no_commit)
list = 1;
- if (!!delete + !!rename + !!copy + !!new_upstream +
+ if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +
list + unset_upstream > 1)
usage_with_options(builtin_branch_usage, options);
@@ -697,6 +715,9 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
if (!argc)
die(_("branch name required"));
return delete_branches(argc, argv, delete > 1, filter.kind,
quiet);
+ } else if (show_current) {
+ print_current_branch_name();
+ return 0;
} else if (list) {
/* git branch --local also shows HEAD when it is detached */
if ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)
diff
Re: [PATCH] http: give curl version warnings consistently
Hi Junio,
On Thu, 25 Oct 2018, Junio C Hamano wrote:
> When a requested feature cannot be activated because the version of
> cURL library used to build Git with is too old, most of the codepaths
> give a warning like "$Feature is not supported with cURL < $Version",
> marked for l10n. A few of them, however, did not follow that pattern
> and said things like "$Feature is not activated, your curl version is
> too old (>= $Version)", and without marking them for l10n.
>
> Update these to match the style of the majority of warnings and mark
> them for l10n.
>
> Signed-off-by: Junio C Hamano
> ---
I like this patch better than the one I had prepared for v2, so I dropped
it again, and "hit the Submit button".
Ciao,
Dscho
>
> > I have a clean-up suggestion related to this but is orthogonal to
> > this three-patch series (after the fix-up is applied, anyway), which
> > I'll be sending out separately.
>
> So, here it is.
>
> http.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/http.c b/http.c
> index 43e75ac583..2214100e3b 100644
> --- a/http.c
> +++ b/http.c
> @@ -834,8 +834,7 @@ static CURL *get_curl_handle(void)
> #if LIBCURL_VERSION_NUM >= 0x072c00
> curl_easy_setopt(result, CURLOPT_SSL_OPTIONS,
> CURLSSLOPT_NO_REVOKE);
> #else
> - warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options
> because\n"
> - "your curl version is too old (< 7.44.0)");
> + warning(_("CURLSSLOPT_NO_REVOKE not suported with cURL <
> 7.44.0"));
> #endif
> }
>
> @@ -908,8 +907,7 @@ static CURL *get_curl_handle(void)
> curl_easy_setopt(result, CURLOPT_PROTOCOLS,
>get_curl_allowed_protocols(-1));
> #else
> - warning("protocol restrictions not applied to curl redirects because\n"
> - "your curl version is too old (>= 7.19.4)");
> + warning(_("Protocol restrictions not supported with cURL < 7.19.4"));
> #endif
> if (getenv("GIT_CURL_VERBOSE"))
> curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
> --
> 2.19.1-542-gc4df23f792
>
>
Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries
> On Wed, 24 Oct 2018, Johannes Schindelin wrote: > > > On Wed, 24 Oct 2018, Junio C Hamano wrote: > > > > > Jonathan, do you see any issues with the use of lookup_commit() in > > > this change wrt lazy clone? I am wondering what happens when the > > > commit in question is at, an immediate parent of, or an immediate > > > child of a promisor object. I _think_ this change won't make it > > > worse for two features in playing together, but thought that it > > > would be better to double check. > > > > Good point. > > > > Instinctively, I would say that no promised object can be a shallow > > commit. The entire idea of a shallow commit is that it *is* present, but > > none of its parents. I'm envisioning a client repo with a single branch, cloned both with --depth=1 and with --filter=, that has just fetched to the same branch also with --depth=1 resulting in a fast-forward from A to B. If A is B's parent, then A would be known to be promised. (Incidentally, the problem is greater in current Git, because for performance reasons, we do not check promisor status when lazily fetching - so it doesn't matter here whether an object is known to be promised or not.) When pruning shallow and checking the existence of A, this would trigger a fetch for A, which would download all commits and trees reachable from it. It sounds safer to me to use the fast approach in this patch when the repository is not partial, and stick to the slow approach when it is. > > However, I am curious whether there is a better way to check for the > > presence of a local commit? Do we have an API function for that, that I > > missed? (I do not really want to parse the commit, after all, just verify > > that it is not pruned.) > > Okay, I looked around a bit. It seems that there is an > `is_promisor_object(oid)` function in `pu` to see whether an object was > promised. If need be (and I am still not convinced that there is a need), > then we can always add a call to that function to the condition. I don't think there is a need for is_promisor_object() either - as described above, it doesn't completely solve the problem. > Coming back to my question whether there is a better way to check for the > presence of a local commit, I figured that I can use `has_object_file()` > instead of looking up and parsing the commit, as this code does not really > need to verify that the shallow entry refers to a commit, but only that it > refers to a local object. Note that has_object_file() also triggers the lazy fetch if needed, but I agree that it's better because you don't really need to parse the commit. There is the possibility of just checking for loose objects (which does not trigger any lazy fetches), which works for builtin/prune since it only prunes loose objects, but doesn't work in the general case, I guess.
Re: [PATCH v3 3/3] commit-slab: missing definitions and forward declarations (hdr-check)
On 25/10/2018 12:04, Carlo Marcelo Arenas Belón wrote: > struct commmit needs to be defined before commit-slab can generate > working code, object_id should be at least known through a forward > declaration > > Signed-off-by: Carlo Marcelo Arenas Belón > --- > commit-slab-impl.h | 2 ++ > commit-slab.h | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/commit-slab-impl.h b/commit-slab-impl.h > index e352c2f8c1..db7cf3f19b 100644 > --- a/commit-slab-impl.h > +++ b/commit-slab-impl.h > @@ -1,6 +1,8 @@ > #ifndef COMMIT_SLAB_IMPL_H > #define COMMIT_SLAB_IMPL_H > > +#include "commit.h" > + > #define implement_static_commit_slab(slabname, elemtype) \ > implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static) > > diff --git a/commit-slab.h b/commit-slab.h > index 69bf0c807c..722252de61 100644 > --- a/commit-slab.h > +++ b/commit-slab.h > @@ -1,6 +1,8 @@ > #ifndef COMMIT_SLAB_H > #define COMMIT_SLAB_H > > +struct object_id; > + > #include "commit-slab-decl.h" > #include "commit-slab-impl.h" > > Hmm, sorry, I don't see how this patch has anything to do with the other two patches! ;-) Also, I have a patch to fix up the 'commit-reach.h' header (it was part of my original series, just had to update the commit message), which adds these very #includes and forward declarations when _using_ the commit-slab. I haven't tried applying your patches yet, which may answer my questions, so I am a little puzzled. ATB, Ramsay Jones
[PATCH v2 3/3] http: when using Secure Channel, ignore sslCAInfo by default
From: Johannes Schindelin
As of cURL v7.60.0, the Secure Channel backend can use the certificate
bundle provided via `http.sslCAInfo`, but that would override the
Windows Certificate Store. Since this is not desirable by default, let's
tell Git to not ask cURL to use that bundle by default when the `schannel`
backend was configured via `http.sslBackend`, unless
`http.schannelUseSSLCAInfo` overrides this behavior.
Signed-off-by: Johannes Schindelin
---
Documentation/config.txt | 8
http.c | 19 ++-
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index d569ebd49..1f6a6a4a6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1997,6 +1997,14 @@ http.schannelCheckRevoke::
certificate. This option is ignored if cURL lacks support for
setting the relevant SSL option at runtime.
+http.schannelUseSSLCAInfo::
+ As of cURL v7.60.0, the Secure Channel backend can use the
+ certificate bundle provided via `http.sslCAInfo`, but that would
+ override the Windows Certificate Store. Since this is not desirable
+ by default, Git will tell cURL not to use that bundle by default
+ when the `schannel` backend was configured via `http.sslBackend`,
+ unless `http.schannelUseSSLCAInfo` overrides this behavior.
+
http.pinnedpubkey::
Public key of the https service. It may either be the filename of
a PEM or DER encoded public key file or a string starting with
diff --git a/http.c b/http.c
index 65daa9bfa..28009ca73 100644
--- a/http.c
+++ b/http.c
@@ -158,6 +158,12 @@ static char *cached_accept_language;
static char *http_ssl_backend;
static int http_schannel_check_revoke = 1;
+/*
+ * With the backend being set to `schannel`, setting sslCAinfo would override
+ * the Certificate Store in cURL v7.60.0 and later, which is not what we want
+ * by default.
+ */
+static int http_schannel_use_ssl_cainfo;
size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
{
@@ -317,6 +323,11 @@ static int http_options(const char *var, const char
*value, void *cb)
return 0;
}
+ if (!strcmp("http.schannelusesslcainfo", var)) {
+ http_schannel_use_ssl_cainfo = git_config_bool(var, value);
+ return 0;
+ }
+
if (!strcmp("http.minsessions", var)) {
min_curl_sessions = git_config_int(var, value);
#ifndef USE_CURL_MULTI
@@ -869,7 +880,13 @@ static CURL *get_curl_handle(void)
if (ssl_pinnedkey != NULL)
curl_easy_setopt(result, CURLOPT_PINNEDPUBLICKEY,
ssl_pinnedkey);
#endif
- if (ssl_cainfo != NULL)
+ if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
+ !http_schannel_use_ssl_cainfo) {
+ curl_easy_setopt(result, CURLOPT_CAINFO, NULL);
+#if LIBCURL_VERSION_NUM >= 0x073400
+ curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, NULL);
+#endif
+ } else if (ssl_cainfo != NULL)
curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
if (curl_low_speed_limit > 0 && curl_low_speed_time > 0) {
--
gitgitgadget
[PATCH v2 2/3] http: add support for disabling SSL revocation checks in cURL
From: Brendan Forster
This adds support for a new http.schannelCheckRevoke config value.
This config value is only used if http.sslBackend is set to "schannel",
which forces cURL to use the Windows Certificate Store when validating
server certificates associated with a remote server.
This config value should only be set to "false" if you are in an
environment where revocation checks are blocked by the network, with
no alternative options.
This is only supported in cURL 7.44 or later.
Note: originally, we wanted to call the config setting
`http.schannel.checkRevoke`. This, however, does not work: the `http.*`
config settings can be limited to specific URLs via `http..*`
(and this feature would mistake `schannel` for a URL).
Helped by Agustín Martín Barbero.
Signed-off-by: Brendan Forster
Signed-off-by: Johannes Schindelin
---
Documentation/config.txt | 8
http.c | 17 +
2 files changed, 25 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7d38f0bf1..d569ebd49 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1989,6 +1989,14 @@ http.sslBackend::
This option is ignored if cURL lacks support for choosing the SSL
backend at runtime.
+http.schannelCheckRevoke::
+ Used to enforce or disable certificate revocation checks in cURL
+ when http.sslBackend is set to "schannel". Defaults to `true` if
+ unset. Only necessary to disable this if Git consistently errors
+ and the message is about checking the revocation status of a
+ certificate. This option is ignored if cURL lacks support for
+ setting the relevant SSL option at runtime.
+
http.pinnedpubkey::
Public key of the https service. It may either be the filename of
a PEM or DER encoded public key file or a string starting with
diff --git a/http.c b/http.c
index 7fb37a061..65daa9bfa 100644
--- a/http.c
+++ b/http.c
@@ -157,6 +157,8 @@ static char *cached_accept_language;
static char *http_ssl_backend;
+static int http_schannel_check_revoke = 1;
+
size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
{
size_t size = eltsize * nmemb;
@@ -310,6 +312,11 @@ static int http_options(const char *var, const char
*value, void *cb)
return 0;
}
+ if (!strcmp("http.schannelcheckrevoke", var)) {
+ http_schannel_check_revoke = git_config_bool(var, value);
+ return 0;
+ }
+
if (!strcmp("http.minsessions", var)) {
min_curl_sessions = git_config_int(var, value);
#ifndef USE_CURL_MULTI
@@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
}
#endif
+ if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
+ !http_schannel_check_revoke) {
+#if LIBCURL_VERSION_NUM >= 0x072c00
+ curl_easy_setopt(result, CURLOPT_SSL_OPTIONS,
CURLSSLOPT_NO_REVOKE);
+#else
+ warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options
because\n"
+ "your curl version is too old (< 7.44.0)");
+#endif
+ }
+
if (http_proactive_auth)
init_curl_http_auth(result);
--
gitgitgadget
[PATCH v2 1/3] http: add support for selecting SSL backends at runtime
From: Johannes Schindelin
As of version 7.56.0, curl supports being compiled with multiple SSL
backends.
This patch adds the Git side of that feature: by setting http.sslBackend
to "openssl" or "schannel", Git for Windows can now choose the SSL
backend at runtime.
This comes in handy on Windows because Secure Channel ("schannel") is
the native solution, accessing the Windows Credential Store, thereby
allowing for enterprise-wide management of certificates. For historical
reasons, Git for Windows needs to support OpenSSL still, as it has
previously been the only supported SSL backend in Git for Windows for
almost a decade.
The patch has been carried in Git for Windows for over a year, and is
considered mature.
Signed-off-by: Johannes Schindelin
---
Documentation/config.txt | 5 +
http.c | 35 +++
2 files changed, 40 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 154683321..7d38f0bf1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1984,6 +1984,11 @@ http.sslCAPath::
with when fetching or pushing over HTTPS. Can be overridden
by the `GIT_SSL_CAPATH` environment variable.
+http.sslBackend::
+ Name of the SSL backend to use (e.g. "openssl" or "schannel").
+ This option is ignored if cURL lacks support for choosing the SSL
+ backend at runtime.
+
http.pinnedpubkey::
Public key of the https service. It may either be the filename of
a PEM or DER encoded public key file or a string starting with
diff --git a/http.c b/http.c
index 98ff12258..7fb37a061 100644
--- a/http.c
+++ b/http.c
@@ -155,6 +155,8 @@ static struct active_request_slot *active_queue_head;
static char *cached_accept_language;
+static char *http_ssl_backend;
+
size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
{
size_t size = eltsize * nmemb;
@@ -302,6 +304,12 @@ static int http_options(const char *var, const char
*value, void *cb)
curl_ssl_try = git_config_bool(var, value);
return 0;
}
+ if (!strcmp("http.sslbackend", var)) {
+ free(http_ssl_backend);
+ http_ssl_backend = xstrdup_or_null(value);
+ return 0;
+ }
+
if (!strcmp("http.minsessions", var)) {
min_curl_sessions = git_config_int(var, value);
#ifndef USE_CURL_MULTI
@@ -995,6 +1003,33 @@ void http_init(struct remote *remote, const char *url,
int proactive_auth)
git_config(urlmatch_config_entry, );
free(normalized_url);
+#if LIBCURL_VERSION_NUM >= 0x073800
+ if (http_ssl_backend) {
+ const curl_ssl_backend **backends;
+ struct strbuf buf = STRBUF_INIT;
+ int i;
+
+ switch (curl_global_sslset(-1, http_ssl_backend, )) {
+ case CURLSSLSET_UNKNOWN_BACKEND:
+ strbuf_addf(, _("Unsupported SSL backend '%s'. "
+ "Supported SSL backends:"),
+ http_ssl_backend);
+ for (i = 0; backends[i]; i++)
+ strbuf_addf(, "\n\t%s", backends[i]->name);
+ die("%s", buf.buf);
+ case CURLSSLSET_NO_BACKENDS:
+ die(_("Could not set SSL backend to '%s': "
+ "cURL was built without SSL backends"),
+ http_ssl_backend);
+ case CURLSSLSET_TOO_LATE:
+ die(_("Could not set SSL backend to '%s': already set"),
+ http_ssl_backend);
+ case CURLSSLSET_OK:
+ break; /* Okay! */
+ }
+ }
+#endif
+
if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
die("curl_global_init failed");
--
gitgitgadget
[PATCH v2 0/3] Allow choosing the SSL backend cURL uses (plus related patches)
This topic branch brings support for choosing cURL's SSL backend (a feature
developed in Git for Windows' context) at runtime via http.sslBackend, and
two more patches that are related (and only of interest for Windows users).
Changes since v1:
* Reworded the commit message of v1's patch 2/3, to talk about the original
design instead of "an earlier iteration" that was never contributed to
the Git mailing list.
* Changed the confusing >= 7.44.0 to < 7.44.0.
Note: I had prepared
https://github.com/dscho/git/commit/81e8c9a4006c919747a0b6a287f28f25799fcaf4
, intended to be included in v2, but Junio came up with
https://public-inbox.org/git/xmqqsh0uln5c.fsf...@gitster-ct.c.googlers.com/
in the meantime, which I like better.
Brendan Forster (1):
http: add support for disabling SSL revocation checks in cURL
Johannes Schindelin (2):
http: add support for selecting SSL backends at runtime
http: when using Secure Channel, ignore sslCAInfo by default
Documentation/config.txt | 21
http.c | 71 +++-
2 files changed, 91 insertions(+), 1 deletion(-)
base-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-46%2Fdscho%2Fhttp-ssl-backend-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-46/dscho/http-ssl-backend-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/46
Range-diff vs v1:
1: 8c5ecdb6c = 1: 85bd0fb27 http: add support for selecting SSL backends at
runtime
2: 764791d13 ! 2: 951383695 http: add support for disabling SSL revocation
checks in cURL
@@ -14,10 +14,10 @@
This is only supported in cURL 7.44 or later.
-Note: an earlier iteration tried to use the config setting
-http.schannel.checkRevoke, but the http.* config settings can be
limited
-to specific URLs via http..* (which would mistake `schannel` for
a
-URL).
+Note: originally, we wanted to call the config setting
+`http.schannel.checkRevoke`. This, however, does not work: the
`http.*`
+config settings can be limited to specific URLs via `http..*`
+(and this feature would mistake `schannel` for a URL).
Helped by Agustín Martín Barbero.
@@ -77,7 +77,7 @@
+ curl_easy_setopt(result, CURLOPT_SSL_OPTIONS,
CURLSSLOPT_NO_REVOKE);
+#else
+ warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options
because\n"
-+ "your curl version is too old (>= 7.44.0)");
++ "your curl version is too old (< 7.44.0)");
+#endif
+ }
+
3: 9927e4ce6 = 3: a5f937a36 http: when using Secure Channel, ignore
sslCAInfo by default
--
gitgitgadget
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
Hi Junio, On Thu, 18 Oct 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > >> In any case, you can use "http..$variable" to say "I want the > >> http.$variable to be in effect but only when I am talking to ". > >> Does it make sense for this new variable, too? That is, does it > >> benefit the users to be able to do something like this? > >> > >> [http] schannelCheckRevoke = no > >> [http "https://microsoft.com/;] schannelCheckRevoke = yes > >> > >> I am guessing that the answer is yes. > > > > Frankly, I do not know. Does it hurt, though? > > I did not and I do not think it would. I was wondering if the > ability to be able to specify these per destination is something > very useful and deserves to be called out in the doc, together with > ... I do not think that it needs to be called out specifically in the docs. It is just yet another http.* setting that can be overridden per-URL. It would be different if it had not worked. > >> I guess the same comment applies to the previous step, but I suspect > >> that the code structure may not allow us to switch the SSL backend > >> so late in the game (e.g. "when talking to microsoft, use schannel, > >> but when talking to github, use openssl"). > > ... this bit. > > > Crucially, this fails. The short version is: this is good! Because it > > means that Git used the OpenSSL backend, as clearly intended. > > > > > > Why does it fail? > > ... > > > > So there may still be some polishing needed, but as long as people > are not using the "per destination" thing, the code is already good? > That is something we may want to document. Actually, just because there is a peculiar bug in this particular code flow in Git for Windows should not necessarily be interesting to Git's commit history. On Linux, for example, it would work. So I don't think we need to mention anything about that unrelated bug. Thanks, Dscho > Thanks. >
Re: [PATCH v7 00/10] Make submodules work if .gitmodules is not checked out
> In this series I am addressing the comments by Stefan Beller about the > tests in patch 9. > > If the new tests look OK, I'd say we try moving the series to "next" and > see what happens? Sounds good to me. Thanks, Stefan
[RFC PATCH] index-pack: improve performance on NFS
The index-pack command determines if a sha1 collision test is needed by
checking the existence of a loose object with the given hash. In my tests, I
can improve performance of “git clone” on Amazon EFS by 8x when used with a
non-default mount option (lookupcache=pos) that's required for a Gitlab HA
setup. My assumption is that this check is unnecessary when cloning into a new
repository because the repository will be empty.
By default, the Linux NFS client will cache directory entries as well as the
non-existence of directory entries. The latter means that when client c1 does
stat() on a file that does not exist, the non-existence will be cached and any
subsequent stat() operation on the file will return -ENOENT until the cache
expires or is invalidated, even if the file was created on client c2 in the
mean time. This leads to errors in a Gitlab HA setup when it distributes jobs
over multiple worker nodes assuming each worker node has the same view of the
shared file system.
The recommended workaround by Gitlab is to use the “lookupcache=pos” NFS mount
option which disables the negative lookup cache. This option has a high
performance impact. Cloning the gitlab-ce repository
(https://gitlab.com/gitlab-org/gitlib-ce.git) into an NFS mounted directory
gives the following results:
lookupcache=all (default): 624 seconds
lookupcache=pos: 4957 seconds
The reason for the poor performance is that index-pack will issue a stat()
call for every object in the repo when checking if a collision test is needed.
These stat() calls result in the following NFS operations:
LOOKUP dirfh=".git/objects", name="01" -> NFS4ERR_ENOENT
With lookupcache=all, the non-existence of the .git/objects/XX directories is
cached, so that there will be at most 256 LOOKUP calls. With lookupcache=pos,
there will be one LOOKUP operation for every object in the repository, which
in case of the gitlab-ce repo is about 1.3 million times.
The attached patch removes the collision check when cloning into a new
repository. The performance of git clone with this patch is:
lookupcache=pos (with patch): 577 seconds
I'd welcome feedback on the attached patch and whether my assumption that the
sha1 collision check can be safely omitted when cloning into a new repository
is correct.
Signed-off-by: Geert Jansen
---
builtin/index-pack.c | 5 -
fetch-pack.c | 2 ++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2004e25da..22b3d40fb 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -84,6 +84,7 @@ static int verbose;
static int show_resolving_progress;
static int show_stat;
static int check_self_contained_and_connected;
+static int cloning;
static struct progress *progress;
@@ -794,7 +795,7 @@ static void sha1_object(const void *data, struct
object_entry *obj_entry,
assert(data || obj_entry);
- if (startup_info->have_repository) {
+ if (startup_info->have_repository && !cloning) {
read_lock();
collision_test_needed =
has_sha1_file_with_flags(oid->hash, OBJECT_INFO_QUICK);
@@ -1705,6 +1706,8 @@ int cmd_index_pack(int argc, const char **argv, const
char *prefix)
check_self_contained_and_connected = 1;
} else if (!strcmp(arg, "--fsck-objects")) {
do_fsck_object = 1;
+ } else if (!strcmp(arg, "--cloning")) {
+ cloning = 1;
} else if (!strcmp(arg, "--verify")) {
verify = 1;
} else if (!strcmp(arg, "--verify-stat")) {
diff --git a/fetch-pack.c b/fetch-pack.c
index b3ed7121b..c75bfb8aa 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -843,6 +843,8 @@ static int get_pack(struct fetch_pack_args *args,
argv_array_push(,
"--check-self-contained-and-connected");
if (args->from_promisor)
argv_array_push(, "--promisor");
+ if (args->cloning)
+ argv_array_pushf(, "--cloning");
}
else {
cmd_name = "unpack-objects";
--
2.19.1.328.g5a0cc8aca.dirty
Re: [PATCH] fetch-pack: be more precise in parsing v2 response
On Thu, Oct 25, 2018 at 2:04 AM Junio C Hamano wrote: > > Junio C Hamano writes: > > > Jonathan Tan writes: > > > >> +GIT_TRACE_PACKET="$(pwd)/log" test_must_fail git -C http_child \ > >> +-c protocol.version=2 \ > >> +fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err && > > > > Because test_must_fail is a shell function, the above is not a > > correct way to say "I want GIT_TRACE_PACKET exported only while this > > thing runs". > > > > I'll squash the following in. > > > > t/t5702-protocol-v2.sh | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh > > index 51009ca391..d58fbfa9e5 100755 > > --- a/t/t5702-protocol-v2.sh > > +++ b/t/t5702-protocol-v2.sh > > @@ -555,7 +555,7 @@ test_expect_success 'when server does not send "ready", > > expect FLUSH' ' > > printf "/acknowledgments/,$ s//0001/" \ > > >"$HTTPD_ROOT_PATH/one-time-sed" && > > > > - GIT_TRACE_PACKET="$(pwd)/log" test_must_fail git -C http_child \ > > + test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" git -C http_child \ > > -c protocol.version=2 \ > > fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err && > > grep "fetch< acknowledgments" log && > > I know it only has been a few days, but is there any other issue > in the patch, anybody? I have reviewed the patch and I think it is good with the squashed change above. Thanks, Stefan
Re: Confusing behavior with ignored submodules and `git commit -a`
On Thu, Oct 25, 2018 at 11:03 AM Michael Forney wrote: > > On 2018-03-16, Michael Forney wrote: > > Hi, > > > > In the past few months have noticed some confusing behavior with > > ignored submodules. I finally got around to bisecting this to commit > > 5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure > > submodules can be added or reset). Uh. :( See the discussion starting at https://public-inbox.org/git/20170725213928.125998-4-bmw...@google.com/ specifically https://public-inbox.org/git/xmqqinieq49v@gitster.mtv.corp.google.com/ > > > > Here is a demonstration of the problem: > > [...] > > Up to here is all expected. Makes sense. > > However, if I go to update `foo.txt` and > > commit with `git commit -a`, changes to inner get recorded > > unexpectedly. What's worse is the shortstat output of `git commit -a`, > > and the diff output of `git show` give no indication that the > > submodule was changed. This is really bad. git-status and git-commit share some code, and we'll populate the commit message with a status output. So it seems reasonable to expect the status and the commit to match, i.e. if status tells me there is no change, then commit should not record the submodule update. > > $ git commit -a -m 'update foo.txt' > > [master 6ec564c] update foo.txt > > 1 file changed, 1 insertion(+), 1 deletion(-) > > $ git show > > commit 6ec564c15ddae099c71f01750b4c434557525653 (HEAD -> master) > > Author: Michael Forney > > Date: Fri Mar 16 20:18:37 2018 -0700 > > > > update foo.txt > > > > diff --git a/foo.txt b/foo.txt > > index d00491f..0cfbf08 100644 > > --- a/foo.txt > > +++ b/foo.txt > > @@ -1 +1 @@ > > -1 > > +2 > > $ > > > > There have been a couple occasions where I accidentally pushed local > > changes to ignored submodules because of this. Since they don't show > > up in the log output, it is difficult to figure out what actually has > > gone wrong. How was it prevented before? Just by git commit -a not picking up the submodule change? > > > > Anyway, since the bisected commit (555680869) only mentions add and > > reset, I'm assuming that this is a regression and not a deliberate > > behavior change. The documentation for submodule..ignore states > > that the setting should only affect `git status` and the diff family. > > In terms of my expectations, I would go further and say it should only > > affect `git status` and diffs against the working tree. > > > > I took a brief look through the relevant sources, and it wasn't clear > > to me how to fix this without accidentally changing the behavior of > > other subcommands. > > > > Any help with this issue is appreciated! I guess reverting that commit is not a good idea now, as I would expect something to break. Maybe looking through the series 614ea03a71 (Merge branch 'bw/submodule-config-cleanup', 2017-08-26) to understand why it happened in the context would be a good start. > I accidentally pushed local changes to ignored submodules again due to this. > > Can anyone confirm whether this is the intended behavior of ignore? If > it is, then at least the documentation needs an update saying that > `commit -a` will commit all submodule changes, even if they are > ignored. The docs say "(but it will nonetheless show up in the output of status and commit when it has been staged)" as well, so that commit sounds like a regression? Stefan
Re: Confusing behavior with ignored submodules and `git commit -a`
On 2018-03-16, Michael Forney wrote: > Hi, > > In the past few months have noticed some confusing behavior with > ignored submodules. I finally got around to bisecting this to commit > 5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure > submodules can be added or reset). > > Here is a demonstration of the problem: > > First some repository initialization with a submodule marked as ignored. > > $ git init inner && git -C inner commit --allow-empty -m 'inner 1' > Initialized empty Git repository in /tmp/inner/.git/ > [master (root-commit) ef55bed] inner 1 > $ git init outer && cd outer > Initialized empty Git repository in /tmp/outer/.git/ > $ git submodule add ../inner > Cloning into '/tmp/outer/inner'... > done. > $ echo 1 > foo.txt && git add foo.txt > $ git commit -m 'outer 1' > [master (root-commit) efeb85c] outer 1 > 3 files changed, 5 insertions(+) > create mode 100644 .gitmodules > create mode 100644 foo.txt > create mode 16 inner > $ git config submodule.inner.ignore all > $ git -C inner commit --allow-empty -m 'inner 2' > [master 7b7f0fa] inner 2 > $ git status > On branch master > nothing to commit, working tree clean > $ > > Up to here is all expected. However, if I go to update `foo.txt` and > commit with `git commit -a`, changes to inner get recorded > unexpectedly. What's worse is the shortstat output of `git commit -a`, > and the diff output of `git show` give no indication that the > submodule was changed. > > $ echo 2 > foo.txt > $ git status > On branch master > Changes not staged for commit: > (use "git add ..." to update what will be committed) > (use "git checkout -- ..." to discard changes in working directory) > > modified: foo.txt > > no changes added to commit (use "git add" and/or "git commit -a") > $ git commit -a -m 'update foo.txt' > [master 6ec564c] update foo.txt > 1 file changed, 1 insertion(+), 1 deletion(-) > $ git show > commit 6ec564c15ddae099c71f01750b4c434557525653 (HEAD -> master) > Author: Michael Forney > Date: Fri Mar 16 20:18:37 2018 -0700 > > update foo.txt > > diff --git a/foo.txt b/foo.txt > index d00491f..0cfbf08 100644 > --- a/foo.txt > +++ b/foo.txt > @@ -1 +1 @@ > -1 > +2 > $ > > There have been a couple occasions where I accidentally pushed local > changes to ignored submodules because of this. Since they don't show > up in the log output, it is difficult to figure out what actually has > gone wrong. > > Anyway, since the bisected commit (555680869) only mentions add and > reset, I'm assuming that this is a regression and not a deliberate > behavior change. The documentation for submodule..ignore states > that the setting should only affect `git status` and the diff family. > In terms of my expectations, I would go further and say it should only > affect `git status` and diffs against the working tree. > > I took a brief look through the relevant sources, and it wasn't clear > to me how to fix this without accidentally changing the behavior of > other subcommands. > > Any help with this issue is appreciated! I accidentally pushed local changes to ignored submodules again due to this. Can anyone confirm whether this is the intended behavior of ignore? If it is, then at least the documentation needs an update saying that `commit -a` will commit all submodule changes, even if they are ignored.
[PATCH] travis-ci: no longer use containers
Travis CI will soon deprecate the container-based infrastructure enabled by `sudo: false` in ce59dffb34190e780be2fa9f449f842cadee9753. More info: https://blog.travis-ci.com/2018-10-04-combining-linux-infrastructures Signed-off-by: Sebastian Staudt --- .travis.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 4d4e26c9df..8d2499739e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,7 +1,5 @@ language: c -sudo: false - cache: directories: - $HOME/travis-cache -- 2.19.1
Re: [PATCH v4 2/3] reset: add new reset.quiet config setting
On 25/10/2018 10:26, Junio C Hamano wrote: > Junio C Hamano writes: > >> To be honest, I find the second sentence in your rewrite even more >> confusing. It reads as if `reset.quiet` configuration variable >> can be used to restore the "show what is yet to be added" >> behaviour, due to the parenthetical mention of the default behaviour >> without any configuration. >> >> The command reports what is yet to be added to the index >> after `reset` by default. It can be made to only report >> errors with the `--quiet` option, or setting `reset.quiet` >> configuration variable to `true` (the latter can be >> overriden with `--no-quiet`). >> >> That may not be much better, though X-<. > > In any case, the comments are getting closer to the bikeshedding > territory, that can be easily addressed incrementally. I am getting > the impression that everbody agrees that the change is desirable, > sufficiently documented and properly implemented. > > Shall we mark it for "Will merge to 'next'" in the what's cooking > report and leave further refinements to incremental updates as > needed? Yeah, the first version gave me a 'huh?' moment (hence the comment), the last version was better and, as you can see, I am no great shakes at wordsmith-ing documentation! ;-) Thanks! ATB, Ramsay Jones
[PATCH v7 03/10] t7411: merge tests 5 and 6
Tests 5 and 6 check for the effects of the same commit, merge the two tests to make it more straightforward to clean things up after the test has finished. The cleanup will be added in a future commit. Signed-off-by: Antonio Ospite --- t/t7411-submodule-config.sh | 18 +- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index 0bde5850ac..f2cd1f4a2c 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -82,29 +82,21 @@ Submodule name: 'a' for path 'b' Submodule name: 'submodule' for path 'submodule' EOF -test_expect_success 'error in one submodule config lets continue' ' +test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' ' (cd super && cp .gitmodules .gitmodules.bak && echo " value = \"" >>.gitmodules && git add .gitmodules && mv .gitmodules.bak .gitmodules && git commit -m "add error" && - test-tool submodule-config \ - HEAD b \ - HEAD submodule \ - >actual && - test_cmp expect_error actual - ) -' - -test_expect_success 'error message contains blob reference' ' - (cd super && sha1=$(git rev-parse HEAD) && test-tool submodule-config \ HEAD b \ HEAD submodule \ - 2>actual_err && - test_i18ngrep "submodule-blob $sha1:.gitmodules" actual_err >/dev/null + >actual \ + 2>actual_stderr && + test_cmp expect_error actual && + test_i18ngrep "submodule-blob $sha1:.gitmodules" actual_stderr >/dev/null ) ' -- 2.19.1
[PATCH v7 08/10] submodule: add a helper to check if it is safe to write to .gitmodules
Introduce a helper function named is_writing_gitmodules_ok() to verify
that the .gitmodules file is safe to write.
The function name follows the scheme of is_staging_gitmodules_ok().
The two symbolic constants GITMODULES_INDEX and GITMODULES_HEAD are used
to get help from the C preprocessor in preventing typos, especially for
future users.
This is in preparation for a future change which teaches git how to read
.gitmodules from the index or from the current branch if the file is not
available in the working tree.
The rationale behind the check is that writing to .gitmodules requires
the file to be present in the working tree, unless a brand new
.gitmodules is being created (in which case the .gitmodules file would
not exist at all: neither in the working tree nor in the index or in the
current branch).
Expose the functionality also via a "submodule-helper config
--check-writeable" command, as git scripts may want to perform the check
before modifying submodules configuration.
Signed-off-by: Antonio Ospite
---
builtin/submodule--helper.c | 24 +++-
cache.h | 2 ++
submodule.c | 18 ++
submodule.h | 1 +
t/t7411-submodule-config.sh | 31 +++
5 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9af6626e32..b272a78801 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2128,6 +2128,28 @@ static int check_name(int argc, const char **argv, const
char *prefix)
static int module_config(int argc, const char **argv, const char *prefix)
{
+ enum {
+ CHECK_WRITEABLE = 1
+ } command = 0;
+
+ struct option module_config_options[] = {
+ OPT_CMDMODE(0, "check-writeable", ,
+ N_("check if it is safe to write to the .gitmodules
file"),
+ CHECK_WRITEABLE),
+ OPT_END()
+ };
+ const char *const git_submodule_helper_usage[] = {
+ N_("git submodule--helper config name [value]"),
+ N_("git submodule--helper config --check-writeable"),
+ NULL
+ };
+
+ argc = parse_options(argc, argv, prefix, module_config_options,
+git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+ if (argc == 1 && command == CHECK_WRITEABLE)
+ return is_writing_gitmodules_ok() ? 0 : -1;
+
/* Equivalent to ACTION_GET in builtin/config.c */
if (argc == 2)
return print_config_from_gitmodules(the_repository, argv[1]);
@@ -2136,7 +2158,7 @@ static int module_config(int argc, const char **argv,
const char *prefix)
if (argc == 3)
return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
- die("submodule--helper config takes 1 or 2 arguments: name [value]");
+ usage_with_options(git_submodule_helper_usage, module_config_options);
}
#define SUPPORT_SUPER_PREFIX (1<<0)
diff --git a/cache.h b/cache.h
index 59c8a93046..4c1f827c54 100644
--- a/cache.h
+++ b/cache.h
@@ -486,6 +486,8 @@ static inline enum object_type object_type(unsigned int
mode)
#define INFOATTRIBUTES_FILE "info/attributes"
#define ATTRIBUTE_MACRO_PREFIX "[attr]"
#define GITMODULES_FILE ".gitmodules"
+#define GITMODULES_INDEX ":.gitmodules"
+#define GITMODULES_HEAD "HEAD:.gitmodules"
#define GIT_NOTES_REF_ENVIRONMENT "GIT_NOTES_REF"
#define GIT_NOTES_DEFAULT_REF "refs/notes/commits"
#define GIT_NOTES_DISPLAY_REF_ENVIRONMENT "GIT_NOTES_DISPLAY_REF"
diff --git a/submodule.c b/submodule.c
index 24a49eae61..9516685478 100644
--- a/submodule.c
+++ b/submodule.c
@@ -51,6 +51,24 @@ int is_gitmodules_unmerged(const struct index_state *istate)
return 0;
}
+/*
+ * Check if the .gitmodules file is safe to write.
+ *
+ * Writing to the .gitmodules file requires that the file exists in the
+ * working tree or, if it doesn't, that a brand new .gitmodules file is going
+ * to be created (i.e. it's neither in the index nor in the current branch).
+ *
+ * It is not safe to write to .gitmodules if it's not in the working tree but
+ * it is in the index or in the current branch, because writing new values
+ * (and staging them) would blindly overwrite ALL the old content.
+ */
+int is_writing_gitmodules_ok(void)
+{
+ struct object_id oid;
+ return file_exists(GITMODULES_FILE) ||
+ (get_oid(GITMODULES_INDEX, ) < 0 &&
get_oid(GITMODULES_HEAD, ) < 0);
+}
+
/*
* Check if the .gitmodules file has unstaged modifications. This must be
* checked before allowing modifications to the .gitmodules file with the
diff --git a/submodule.h b/submodule.h
index 4826601ff2..ea6e115f16 100644
--- a/submodule.h
+++ b/submodule.h
@@ -40,6 +40,7 @@ struct submodule_update_strategy {
#define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
int
[PATCH v7 10/10] t/helper: add test-submodule-nested-repo-config
Add a test tool to exercise config_from_gitmodules(), in particular for
the case of nested submodules.
Add also a test to document that reading the submoudles config of nested
submodules does not work yet when the .gitmodules file is not in the
working tree but it still in the index.
This is because the git API does not always make it possible access the
object store of an arbitrary repository (see get_oid() usage in
config_from_gitmodules()).
When this git limitation gets fixed the aforementioned use case will be
supported too.
Signed-off-by: Antonio Ospite
---
Makefile | 1 +
t/helper/test-submodule-nested-repo-config.c | 30 +
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t7411-submodule-config.sh | 34
5 files changed, 67 insertions(+)
create mode 100644 t/helper/test-submodule-nested-repo-config.c
diff --git a/Makefile b/Makefile
index d18ab0fe78..d8f4dfdb3e 100644
--- a/Makefile
+++ b/Makefile
@@ -741,6 +741,7 @@ TEST_BUILTINS_OBJS += test-sigchain.o
TEST_BUILTINS_OBJS += test-strcmp-offset.o
TEST_BUILTINS_OBJS += test-string-list.o
TEST_BUILTINS_OBJS += test-submodule-config.o
+TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o
TEST_BUILTINS_OBJS += test-subprocess.o
TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
TEST_BUILTINS_OBJS += test-wildmatch.o
diff --git a/t/helper/test-submodule-nested-repo-config.c
b/t/helper/test-submodule-nested-repo-config.c
new file mode 100644
index 00..a31e2a9bea
--- /dev/null
+++ b/t/helper/test-submodule-nested-repo-config.c
@@ -0,0 +1,30 @@
+#include "test-tool.h"
+#include "submodule-config.h"
+
+static void die_usage(int argc, const char **argv, const char *msg)
+{
+ fprintf(stderr, "%s\n", msg);
+ fprintf(stderr, "Usage: %s \n", argv[0]);
+ exit(1);
+}
+
+int cmd__submodule_nested_repo_config(int argc, const char **argv)
+{
+ struct repository submodule;
+
+ if (argc < 3)
+ die_usage(argc, argv, "Wrong number of arguments.");
+
+ setup_git_directory();
+
+ if (repo_submodule_init(, the_repository, argv[1])) {
+ die_usage(argc, argv, "Submodule not found.");
+ }
+
+ /* Read the config of _child_ submodules. */
+ print_config_from_gitmodules(, argv[2]);
+
+ submodule_free(the_repository);
+
+ return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 6b5836dc1b..ca5c5ede6c 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -46,6 +46,7 @@ static struct test_cmd cmds[] = {
{ "strcmp-offset", cmd__strcmp_offset },
{ "string-list", cmd__string_list },
{ "submodule-config", cmd__submodule_config },
+ { "submodule-nested-repo-config", cmd__submodule_nested_repo_config },
{ "subprocess", cmd__subprocess },
{ "urlmatch-normalization", cmd__urlmatch_normalization },
{ "wildmatch", cmd__wildmatch },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index e4890566da..500e3684e1 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -42,6 +42,7 @@ int cmd__sigchain(int argc, const char **argv);
int cmd__strcmp_offset(int argc, const char **argv);
int cmd__string_list(int argc, const char **argv);
int cmd__submodule_config(int argc, const char **argv);
+int cmd__submodule_nested_repo_config(int argc, const char **argv);
int cmd__subprocess(int argc, const char **argv);
int cmd__urlmatch_normalization(int argc, const char **argv);
int cmd__wildmatch(int argc, const char **argv);
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 2cfabb18bc..89690b7adb 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -216,4 +216,38 @@ test_expect_success 'reading submodules config from the
current branch when .git
)
'
+test_expect_success 'reading nested submodules config' '
+ (cd super &&
+ git init submodule/nested_submodule &&
+ echo "a" >submodule/nested_submodule/a &&
+ git -C submodule/nested_submodule add a &&
+ git -C submodule/nested_submodule commit -m "add a" &&
+ git -C submodule submodule add ./nested_submodule &&
+ git -C submodule add nested_submodule &&
+ git -C submodule commit -m "added nested_submodule" &&
+ git add submodule &&
+ git commit -m "updated submodule" &&
+ echo "./nested_submodule" >expect &&
+ test-tool submodule-nested-repo-config \
+ submodule submodule.nested_submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
+# When this test eventually passes, before turning it into
+# test_expect_success, remember to replace the test_i18ngrep below with
+# a "test_must_be_empty warning" to be sure that the
[PATCH v7 02/10] submodule: factor out a config_set_in_gitmodules_file_gently function
Introduce a new config_set_in_gitmodules_file_gently() function to write
config values to the .gitmodules file.
This is in preparation for a future change which will use the function
to write to the .gitmodules file in a more controlled way instead of
using "git config -f .gitmodules".
The purpose of the change is mainly to centralize the code that writes
to the .gitmodules file to avoid some duplication.
The naming follows git_config_set_in_file_gently() but the git_ prefix
is removed to communicate that this is not a generic git-config API.
Signed-off-by: Antonio Ospite
---
submodule-config.c | 12
submodule-config.h | 1 +
submodule.c| 10 +++---
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index e36d4e2271..329c0b44f6 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -717,6 +717,18 @@ int print_config_from_gitmodules(struct repository *repo,
const char *key)
return 0;
}
+int config_set_in_gitmodules_file_gently(const char *key, const char *value)
+{
+ int ret;
+
+ ret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);
+ if (ret < 0)
+ /* Maybe the user already did that, don't error out here */
+ warning(_("Could not update .gitmodules entry %s"), key);
+
+ return ret;
+}
+
struct fetch_config {
int *max_children;
int *recurse_submodules;
diff --git a/submodule-config.h b/submodule-config.h
index 031747ccf8..4dc9b0771c 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -49,6 +49,7 @@ const struct submodule *submodule_from_path(struct repository
*r,
const char *path);
void submodule_free(struct repository *r);
int print_config_from_gitmodules(struct repository *repo, const char *key);
+int config_set_in_gitmodules_file_gently(const char *key, const char *value);
/*
* Returns 0 if the name is syntactically acceptable as a submodule "name"
diff --git a/submodule.c b/submodule.c
index d9d3046689..24a49eae61 100644
--- a/submodule.c
+++ b/submodule.c
@@ -89,6 +89,7 @@ int update_path_in_gitmodules(const char *oldpath, const char
*newpath)
{
struct strbuf entry = STRBUF_INIT;
const struct submodule *submodule;
+ int ret;
if (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */
return -1;
@@ -104,14 +105,9 @@ int update_path_in_gitmodules(const char *oldpath, const
char *newpath)
strbuf_addstr(, "submodule.");
strbuf_addstr(, submodule->name);
strbuf_addstr(, ".path");
- if (git_config_set_in_file_gently(GITMODULES_FILE, entry.buf, newpath)
< 0) {
- /* Maybe the user already did that, don't error out here */
- warning(_("Could not update .gitmodules entry %s"), entry.buf);
- strbuf_release();
- return -1;
- }
+ ret = config_set_in_gitmodules_file_gently(entry.buf, newpath);
strbuf_release();
- return 0;
+ return ret;
}
/*
--
2.19.1
[PATCH v7 06/10] submodule: use the 'submodule--helper config' command
Use the 'submodule--helper config' command in git-submodules.sh to avoid
referring explicitly to .gitmodules by the hardcoded file path.
This makes it possible to access the submodules configuration in a more
controlled way.
Signed-off-by: Antonio Ospite
---
git-submodule.sh | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 1b568e29b9..0805fadf47 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -72,7 +72,7 @@ get_submodule_config () {
value=$(git config submodule."$name"."$option")
if test -z "$value"
then
- value=$(git config -f .gitmodules submodule."$name"."$option")
+ value=$(git submodule--helper config
submodule."$name"."$option")
fi
printf '%s' "${value:-$default}"
}
@@ -283,11 +283,11 @@ or you are unsure what this means choose another name
with the '--name' option."
git add --no-warn-embedded-repo $force "$sm_path" ||
die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
- git config -f .gitmodules submodule."$sm_name".path "$sm_path" &&
- git config -f .gitmodules submodule."$sm_name".url "$repo" &&
+ git submodule--helper config submodule."$sm_name".path "$sm_path" &&
+ git submodule--helper config submodule."$sm_name".url "$repo" &&
if test -n "$branch"
then
- git config -f .gitmodules submodule."$sm_name".branch "$branch"
+ git submodule--helper config submodule."$sm_name".branch
"$branch"
fi &&
git add --force .gitmodules ||
die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
--
2.19.1
[PATCH v7 01/10] submodule: add a print_config_from_gitmodules() helper
Add a new print_config_from_gitmodules() helper function to print values
from .gitmodules just like "git config -f .gitmodules" would.
This will be used by a new submodule--helper subcommand to be able to
access the .gitmodules file in a more controlled way.
Signed-off-by: Antonio Ospite
---
submodule-config.c | 25 +
submodule-config.h | 1 +
2 files changed, 26 insertions(+)
diff --git a/submodule-config.c b/submodule-config.c
index b132f7a80b..e36d4e2271 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -692,6 +692,31 @@ void submodule_free(struct repository *r)
submodule_cache_clear(r->submodule_cache);
}
+static int config_print_callback(const char *var, const char *value, void
*cb_data)
+{
+ char *wanted_key = cb_data;
+
+ if (!strcmp(wanted_key, var))
+ printf("%s\n", value);
+
+ return 0;
+}
+
+int print_config_from_gitmodules(struct repository *repo, const char *key)
+{
+ int ret;
+ char *store_key;
+
+ ret = git_config_parse_key(key, _key, NULL);
+ if (ret < 0)
+ return CONFIG_INVALID_KEY;
+
+ config_from_gitmodules(config_print_callback, repo, store_key);
+
+ free(store_key);
+ return 0;
+}
+
struct fetch_config {
int *max_children;
int *recurse_submodules;
diff --git a/submodule-config.h b/submodule-config.h
index dc7278eea4..031747ccf8 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -48,6 +48,7 @@ const struct submodule *submodule_from_path(struct repository
*r,
const struct object_id
*commit_or_tree,
const char *path);
void submodule_free(struct repository *r);
+int print_config_from_gitmodules(struct repository *repo, const char *key);
/*
* Returns 0 if the name is syntactically acceptable as a submodule "name"
--
2.19.1
[PATCH v7 07/10] t7506: clean up .gitmodules properly before setting up new scenario
In t/t7506-status-submodule.sh at some point a new scenario is set up to test different things, in particular new submodules are added which are meant to completely replace the previous ones. However before calling the "git submodule add" commands for the new layout, the .gitmodules file is removed only from the working tree still leaving the previous content in current branch. This can break if, in the future, "git submodule add" starts differentiating between the following two cases: - .gitmodules is not in the working tree but it is in the current branch (it may not be safe to add new submodules in this case); - .gitmodules is neither in the working tree nor anywhere in the current branch (it is safe to add new submodules). Since the test intends to get rid of .gitmodules anyways, let's completely remove it from the current branch, to actually start afresh in the new scenario. This is more future-proof and does not break current tests. Signed-off-by: Antonio Ospite --- t/t7506-status-submodule.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh index 943708fb04..08629a6e70 100755 --- a/t/t7506-status-submodule.sh +++ b/t/t7506-status-submodule.sh @@ -325,7 +325,8 @@ test_expect_success 'setup superproject with untracked file in nested submodule' ( cd super && git clean -dfx && - rm .gitmodules && + git rm .gitmodules && + git commit -m "remove .gitmodules" && git submodule add -f ./sub1 && git submodule add -f ./sub2 && git submodule add -f ./sub1 sub3 && -- 2.19.1
[PATCH v7 04/10] t7411: be nicer to future tests and really clean things up
Tests 5 and 7 in t/t7411-submodule-config.sh add two commits with invalid lines in .gitmodules but then only the second commit is removed. This may affect future subsequent tests if they assume that the .gitmodules file has no errors. Remove both the commits as soon as they are not needed anymore. Signed-off-by: Antonio Ospite --- t/t7411-submodule-config.sh | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index f2cd1f4a2c..b1f3c6489b 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -83,6 +83,8 @@ Submodule name: 'submodule' for path 'submodule' EOF test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' ' + ORIG=$(git -C super rev-parse HEAD) && + test_when_finished "git -C super reset --hard $ORIG" && (cd super && cp .gitmodules .gitmodules.bak && echo " value = \"" >>.gitmodules && @@ -115,6 +117,8 @@ test_expect_success 'using different treeishs works' ' ' test_expect_success 'error in history in fetchrecursesubmodule lets continue' ' + ORIG=$(git -C super rev-parse HEAD) && + test_when_finished "git -C super reset --hard $ORIG" && (cd super && git config -f .gitmodules \ submodule.submodule.fetchrecursesubmodules blabla && @@ -126,8 +130,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' ' HEAD b \ HEAD submodule \ >actual && - test_cmp expect_error actual && - git reset --hard HEAD^ + test_cmp expect_error actual ) ' -- 2.19.1
[PATCH v7 00/10] Make submodules work if .gitmodules is not checked out
Hi, this series teaches git to try and read the .gitmodules file from the index (:.gitmodules) or from the current branch (HEAD:.gitmodules) when the file is not readily available in the working tree. This can be used, together with sparse checkouts, to enable submodule usage with programs like vcsh[1] which manage multiple repositories with their working trees sharing the same path. [1] https://github.com/RichiH/vcsh In the previous series there were some comments about not using the enum in patch 8, but I decided to leave the code as it was as I still think that it make sense to use an enum there, and have the default value unnamed; during the discussion I pointed out that other code in git do the same. In this series I am addressing the comments by Stefan Beller about the tests in patch 9. If the new tests look OK, I'd say we try moving the series to "next" and see what happens? I am available to address any further concerns in follow-up patches. v6 of the series is here: https://public-inbox.org/git/20181005130601.15879-1-...@ao2.it/ v5 of the series is here: https://public-inbox.org/git/20180917140940.3839-1-...@ao2.it/ v4 of the series is here: https://public-inbox.org/git/20180824132951.8000-1-...@ao2.it/ v3 of the series is here: https://public-inbox.org/git/20180814110525.17801-1-...@ao2.it/ v2 of the series is here: https://public-inbox.org/git/20180802134634.10300-1-...@ao2.it/ v1 of the series, with some background info, is here: https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/ Changes since v6: * Renamed t7416-submodule-sparse-gitmodules.sh to t7418-submodule-sparse-gitmodules.sh because t7416 was already taken. This has been already taken care of by Junio in "pu". * Improved tests in t7418: now, instead of just testing the return value of "git submodule ..." commands when .gitmodules is not in the working tree, the actual use case is checked in each test, with pre- and post-conditions. Thank you, Antonio Antonio Ospite (10): submodule: add a print_config_from_gitmodules() helper submodule: factor out a config_set_in_gitmodules_file_gently function t7411: merge tests 5 and 6 t7411: be nicer to future tests and really clean things up submodule--helper: add a new 'config' subcommand submodule: use the 'submodule--helper config' command t7506: clean up .gitmodules properly before setting up new scenario submodule: add a helper to check if it is safe to write to .gitmodules submodule: support reading .gitmodules when it's not in the working tree t/helper: add test-submodule-nested-repo-config Makefile | 1 + builtin/grep.c | 17 ++- builtin/submodule--helper.c | 40 ++ cache.h | 2 + git-submodule.sh | 13 +- submodule-config.c | 68 - submodule-config.h | 2 + submodule.c | 28 +++- submodule.h | 1 + t/helper/test-submodule-nested-repo-config.c | 30 t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t7411-submodule-config.sh | 141 +-- t/t7418-submodule-sparse-gitmodules.sh | 122 t/t7506-status-submodule.sh | 3 +- t/t7814-grep-recurse-submodules.sh | 16 +++ 16 files changed, 454 insertions(+), 32 deletions(-) create mode 100644 t/helper/test-submodule-nested-repo-config.c create mode 100755 t/t7418-submodule-sparse-gitmodules.sh -- Antonio Ospite https://ao2.it https://twitter.com/ao2it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing?
[PATCH v7 09/10] submodule: support reading .gitmodules when it's not in the working tree
When the .gitmodules file is not available in the working tree, try
using the content from the index and from the current branch. This
covers the case when the file is part of the repository but for some
reason it is not checked out, for example because of a sparse checkout.
This makes it possible to use at least the 'git submodule' commands
which *read* the gitmodules configuration file without fully populating
the working tree.
Writing to .gitmodules will still require that the file is checked out,
so check for that before calling config_set_in_gitmodules_file_gently.
Add a similar check also in git-submodule.sh::cmd_add() to anticipate
the eventual failure of the "git submodule add" command when .gitmodules
is not safely writeable; this prevents the command from leaving the
repository in a spurious state (e.g. the submodule repository was cloned
but .gitmodules was not updated because
config_set_in_gitmodules_file_gently failed).
Moreover, since config_from_gitmodules() now accesses the global object
store, it is necessary to protect all code paths which call the function
against concurrent access to the global object store. Currently this
only happens in builtin/grep.c::grep_submodules(), so call
grep_read_lock() before invoking code involving
config_from_gitmodules().
Finally, add t7418-submodule-sparse-gitmodules.sh to verify that reading
from .gitmodules succeeds and that writing to it fails when the file is
not checked out.
NOTE: there is one rare case where this new feature does not work
properly yet: nested submodules without .gitmodules in their working
tree. This has been documented with a warning and a test_expect_failure
item in t7814, and in this case the current behavior is not altered: no
config is read.
Signed-off-by: Antonio Ospite
---
builtin/grep.c | 17 +++-
builtin/submodule--helper.c| 6 +-
git-submodule.sh | 5 +
submodule-config.c | 31 ++-
t/t7411-submodule-config.sh| 26 +-
t/t7418-submodule-sparse-gitmodules.sh | 122 +
t/t7814-grep-recurse-submodules.sh | 16
7 files changed, 216 insertions(+), 7 deletions(-)
create mode 100755 t/t7418-submodule-sparse-gitmodules.sh
diff --git a/builtin/grep.c b/builtin/grep.c
index d8508ddf79..56e4a11052 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -422,11 +422,23 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
struct repository submodule;
int hit;
- if (!is_submodule_active(superproject, path))
+ /*
+* NEEDSWORK: submodules functions need to be protected because they
+* access the object store via config_from_gitmodules(): the latter
+* uses get_oid() which, for now, relies on the global the_repository
+* object.
+*/
+ grep_read_lock();
+
+ if (!is_submodule_active(superproject, path)) {
+ grep_read_unlock();
return 0;
+ }
- if (repo_submodule_init(, superproject, path))
+ if (repo_submodule_init(, superproject, path)) {
+ grep_read_unlock();
return 0;
+ }
repo_read_gitmodules();
@@ -440,7 +452,6 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
* store is no longer global and instead is a member of the repository
* object.
*/
- grep_read_lock();
add_to_alternates_memory(submodule.objects->objectdir);
grep_read_unlock();
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b272a78801..05ce666142 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2155,8 +2155,12 @@ static int module_config(int argc, const char **argv,
const char *prefix)
return print_config_from_gitmodules(the_repository, argv[1]);
/* Equivalent to ACTION_SET in builtin/config.c */
- if (argc == 3)
+ if (argc == 3) {
+ if (!is_writing_gitmodules_ok())
+ die(_("please make sure that the .gitmodules file is in
the working tree"));
+
return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+ }
usage_with_options(git_submodule_helper_usage, module_config_options);
}
diff --git a/git-submodule.sh b/git-submodule.sh
index 0805fadf47..f5124bbf23 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -159,6 +159,11 @@ cmd_add()
shift
done
+ if ! git submodule--helper config --check-writeable >/dev/null 2>&1
+ then
+die "$(eval_gettext "please make sure that the .gitmodules
file is in the working tree")"
+ fi
+
if test -n "$reference_path"
then
is_absolute_path "$reference_path" ||
diff --git a/submodule-config.c b/submodule-config.c
index 329c0b44f6..52702c62d9 100644
---
[PATCH v7 05/10] submodule--helper: add a new 'config' subcommand
Add a new 'config' subcommand to 'submodule--helper', this extra level
of indirection makes it possible to add some flexibility to how the
submodules configuration is handled.
Signed-off-by: Antonio Ospite
---
builtin/submodule--helper.c | 14 ++
t/t7411-submodule-config.sh | 27 +++
2 files changed, 41 insertions(+)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 80474c3ff5..9af6626e32 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2126,6 +2126,19 @@ static int check_name(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int module_config(int argc, const char **argv, const char *prefix)
+{
+ /* Equivalent to ACTION_GET in builtin/config.c */
+ if (argc == 2)
+ return print_config_from_gitmodules(the_repository, argv[1]);
+
+ /* Equivalent to ACTION_SET in builtin/config.c */
+ if (argc == 3)
+ return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+
+ die("submodule--helper config takes 1 or 2 arguments: name [value]");
+}
+
#define SUPPORT_SUPER_PREFIX (1<<0)
struct cmd_struct {
@@ -2155,6 +2168,7 @@ static struct cmd_struct commands[] = {
{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
{"is-active", is_active, 0},
{"check-name", check_name, 0},
+ {"config", module_config, 0},
};
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index b1f3c6489b..791245f18d 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -134,4 +134,31 @@ test_expect_success 'error in history in
fetchrecursesubmodule lets continue' '
)
'
+test_expect_success 'reading submodules config with "submodule--helper
config"' '
+ (cd super &&
+ echo "../submodule" >expect &&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'writing submodules config with "submodule--helper
config"' '
+ (cd super &&
+ echo "new_url" >expect &&
+ git submodule--helper config submodule.submodule.url "new_url"
&&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'overwriting unstaged submodules config with
"submodule--helper config"' '
+ test_when_finished "git -C super checkout .gitmodules" &&
+ (cd super &&
+ echo "newer_url" >expect &&
+ git submodule--helper config submodule.submodule.url
"newer_url" &&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
2.19.1
[PATCH v1 1/2] path.c: char is not (always) signed
From: Torsten Bögershausen
If a "char" in C is signed or unsigned is not specified, because it is
out of tradition "implementation dependent".
Therefore constructs like "if (name[i] < 0)" are not portable,
use "if (name[i] & 0x80)" instead.
Detected by "gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516" when
setting
DEVELOPER = 1
DEVOPTS = extra-all
Signed-off-by: Torsten Bögershausen
---
path.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/path.c b/path.c
index 34f0f98349..ba06ec5b2d 100644
--- a/path.c
+++ b/path.c
@@ -1369,7 +1369,7 @@ static int is_ntfs_dot_generic(const char *name,
saw_tilde = 1;
} else if (i >= 6)
return 0;
- else if (name[i] < 0) {
+ else if (name[i] & 0x80) {
/*
* We know our needles contain only ASCII, so we clamp
* here to make the results of tolower() sane.
--
2.11.0
[PATCH v1 2/2] curl_off_t xcurl_off_t is not portable
From: Torsten Bögershausen
Comparing signed and unsigned values is not always portable.
When setting
DEVELOPER = 1
DEVOPTS = extra-all
"gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516" errors out with
"comparison is always false due to limited range of data type"
"[-Werror=type-limits]"
Solution:
Use a valid cast & compare, similar to xsize_t()
Signed-off-by: Torsten Bögershausen
---
remote-curl.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 762a55a75f..c89fd6d1c3 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -618,9 +618,10 @@ static int probe_rpc(struct rpc_state *rpc, struct
slot_results *results)
}
static curl_off_t xcurl_off_t(ssize_t len) {
- if (len > maximum_signed_value_of_type(curl_off_t))
+ curl_off_t size = (curl_off_t) len;
+ if (len != (ssize_t) size)
die("cannot handle pushes this big");
- return (curl_off_t) len;
+ return size;
}
static int post_rpc(struct rpc_state *rpc)
--
2.11.0
Git Gui on OSX Mojave Dark Flashes on Repaints
On the newest version of Git available via Homebrew (2.19.1), when the gui tool needs to repaint the screen (refresh a file, change a focused file, stage files) the window that is repainted flashes a dark gray before returning to white. This was not occurring on High Sierra yesterday, but has occurred today. I believe it was occurring on an older version of Git as well, but I updated to ensure that it wasn't something that was already solved (it was not). I can provide more diagnostic data if needed. Thanks! Matt
Bug: git log changes output depending on how the output is used
I noticed that the piped output still prints the (HEAD -> ) : git log --pretty=format:"%d*%B" --decorate-refs='refs/tags/*' *bar (tag: "123")*foo git log --pretty=format:"%d*%B" --decorate-refs='refs/tags/*' > tmp.tmp (HEAD -> branch)*bar (tag: "123")*foo I would expect the output that is printed to console, not what's present in tmp.tmp
Re: the opposite of .gitignore, whitelist
Hi all, On 10/25/18 1:37 AM, Junio C Hamano wrote: > "lhf...@163.com" writes: > >> I have a good idea, add a file to git that is the opposite of .gitignore..., > Do negative patterns in .gitignore file help without inventing > anything new? I did this several years ago in an attempt to track /etc/ (minus ownership, of course) without storing secrets in the git history. As the system grew and was maintained (read: crap added), the negative patterns grew untenable. I quickly realized it wasn't the correct way to solve the problem. Unfortunately, shortly after realizing this, I left that project. So I never had the chance to develop a proper solution. However, the concept of a '.gitonly' file was exactly was I was seeking. So, for what it's worth, I've definitely had at least one legit usecase for this feature. The usecases tend to center around tracking select files within the rootfs of a full-blown operating system. Or a subset thereof. hth, Jason.
[PATCH] Move upstream status from gitstring to f
Upstream status should be spaced even if other statuses don't exist
for consistency of view. Eg: if a repository is freshly cloned, the
prompt shows "(master=)" but with an additional status like a change,
it'll show "(master *=)". Now it'll show "(master =)" and accounts for
other states as well.
Signed-off-by: Khinshan Khan
---
contrib/completion/git-prompt.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 983e419..4715d33 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -517,8 +517,8 @@ __git_ps1 ()
b="\${__git_ps1_branch_name}"
fi
- local f="$w$i$s$u"
- local gitstring="$c$b${f:+$z$f}$r$p"
+ local f="$w$i$s$u$p"
+ local gitstring="$c$b${f:+$z$f}$r"
if [ $pcmode = yes ]; then
if [ "${__git_printf_supports_v-}" != yes ]; then
--
2.7.4
Re: [PATCH v4 2/3] reset: add new reset.quiet config setting
On 10/25/2018 5:26 AM, Junio C Hamano wrote: Junio C Hamano writes: To be honest, I find the second sentence in your rewrite even more confusing. It reads as if `reset.quiet` configuration variable can be used to restore the "show what is yet to be added" behaviour, due to the parenthetical mention of the default behaviour without any configuration. The command reports what is yet to be added to the index after `reset` by default. It can be made to only report errors with the `--quiet` option, or setting `reset.quiet` configuration variable to `true` (the latter can be overriden with `--no-quiet`). That may not be much better, though X-<. In any case, the comments are getting closer to the bikeshedding territory, that can be easily addressed incrementally. I am getting the impression that everbody agrees that the change is desirable, sufficiently documented and properly implemented. Shall we mark it for "Will merge to 'next'" in the what's cooking report and leave further refinements to incremental updates as needed? While not great, I think it is good enough. I don't think either of the last couple of rewrite attempts were clearly better than what is in the latest patch. I'd agree we should merge to 'next' and if someone comes up with something great, we can update it then.
Re: [PATCH v6 00/10] Make submodules work if .gitmodules is not checked out
On Thu, 25 Oct 2018 17:40:47 +0900 Junio C Hamano wrote: > Antonio Ospite writes: > > > this series teaches git to try and read the .gitmodules file from the > > index (:.gitmodules) or from the current branch (HEAD:.gitmodules) when > > the file is not readily available in the working tree. > > What you said in [*1*] the discussion on [09/10] sounded like you > are preparing an update of the series, so the topic is marked as > "Expecting a reroll" in the recent "What's cooking" report. At > least one topic now depends on the enhancement this topic makes, so > I'd like to know what the current status and ETA of the reroll would > be, in order to sort-of act as a traffic cop. > Hi Junio, I can send a v7 later today. It will only contain the improvements to 7416-submodule-sparse-gitmodules.sh as discussed in [*1*], it won't contain changes to patch 8 as motivated in https://public-inbox.org/git/20181008143709.dfcc845ab393c9caea660...@ao2.it/ I will also leave patch 10 unchanged, improvements can be made in follow-up patches. BTW, what is the new topic which depends on this one? Thank you, Antonio > [Reference] > > *1* > http://public-inbox.org/git/20181010205645.e1529eff9099805029b1d...@ao2.it/ -- Antonio Ospite https://ao2.it https://twitter.com/ao2it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing?
Re: [PATCH v3 7/7] revision.c: refactor basic topo-order logic
On 10/25/2018 5:43 AM, Jeff King wrote: On Thu, Oct 11, 2018 at 12:21:44PM -0400, Derrick Stolee wrote: 2. INDEGREE: using the indegree_queue priority queue (ordered by maximizing the generation number), add one to the in- degree of each parent for each commit that is walked. Since we walk in order of decreasing generation number, we know that discovering an in-degree value of 0 means the value for that commit was not initialized, so should be initialized to two. (Recall that in-degree value "1" is what we use to say a commit is ready for output.) As we iterate the parents of a commit during this walk, ensure the EXPLORE walk has walked beyond their generation numbers. I wondered how this would work for INFINITY. We can't know the order of a bunch of INFINITY nodes at all, so we never know when their in-degree values are "done". But if I understand the EXPLORE walk, we'd basically walk all of INFINITY down to something with a real generation number. Is that right? But after that, I'm not totally clear on why we need this INDEGREE walk. The INDEGREE walk is an important element for Kahn's algorithm. The final output order is dictated by peeling commits of "indegree zero" to ensure all children are output before their parents. (Note: since we use literal 0 to mean "uninitialized", we peel commits when the indegree slab has value 1.) This walk replaces the indegree logic from sort_in_topological_order(). That method performs one walk that fills the indegree slab, then another walk that peels the commits with indegree 0 and inserts them into a list. I guess my big question here was: if we have generation numbers, do we need Kahn's algorithm? That is, in a fully populated set of generation numbers (i.e., no INFINITY), we could always just pick a commit with the highest generation number to show. So if we EXPLORE down to a real generation number in phase 1, why do we need to care about INDEGREE anymore? Or am I wrong that we always have a real generation number (i.e., not INFINITY) after EXPLORE? (And if so, why is exploring to a real generation number a bad idea; presumably it's due to a worst-case that goes deeper than we'd otherwise need to here). The issue is that we our final order (used by level 3) is unrelated to generation number. Yes, if we prioritized by generation number then we could output the commits in _some_ order that doesn't violate topological constraints. However, we are asking for a different priority, which is different than the generation number priority. In the case of "--topo-order", we want to output the commits reachable from the second parent of a merge before the commits reachable from the first parent. However, in most cases the generation number of the first parent is higher than the second parent (there are more things in the merge chain than in a small topic that got merged). The INDEGREE is what allows us to know when we can peel these commits while still respecting the priority we want at the end. Thanks, -Stolee
[PATCH] packfile: close multi-pack-index in close_all_packs
Whenever we delete pack-files from the object directory, we need
to also delete the multi-pack-index that may refer to those
objects. Sometimes, this connection is obvious, like during a
repack. Other times, this is less obvious, like when gc calls
a repack command and then does other actions on the objects, like
write a commit-graph file.
The pattern we use to avoid out-of-date in-memory packed_git
structs is to call close_all_packs(). This should also call
close_midx(). Since we already pass an object store to
close_all_packs(), this is a nicely scoped operation.
This fixes a test failure when running t6500-gc.sh with
GIT_TEST_MULTI_PACK_INDEX=1.
Reported-by: Szeder Gábor
Signed-off-by: Derrick Stolee
---
Thanks for the report, Szeder! I think this is the correct fix,
and more likely to be permanent across all builtins that run
auto-GC. I based it on ds/test-multi-pack-index so it could easily
be added on top.
-Stolee
packfile.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/packfile.c b/packfile.c
index 841b36182f..37fcd8f136 100644
--- a/packfile.c
+++ b/packfile.c
@@ -339,6 +339,11 @@ void close_all_packs(struct raw_object_store *o)
BUG("want to close pack marked 'do-not-close'");
else
close_pack(p);
+
+ if (o->multi_pack_index) {
+ close_midx(o->multi_pack_index);
+ o->multi_pack_index = NULL;
+ }
}
/*
base-commit: 0465a50506023df0932fe0534fe6ac6712c0d854
--
2.17.1
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
Hi Junio,
On Thu, 25 Oct 2018, Junio C Hamano wrote:
> Eric Sunshine writes:
>
> > On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
> > wrote:
> >> This config value is only used if http.sslBackend is set to "schannel",
> >> which forces cURL to use the Windows Certificate Store when validating
> >> server certificates associated with a remote server.
> >>
> >> This is only supported in cURL 7.44 or later.
> >> [...]
> >> Signed-off-by: Brendan Forster
> >> ---
> >> diff --git a/http.c b/http.c
> >> @@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
> >> + if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
> >> + !http_schannel_check_revoke) {
> >> +#if LIBCURL_VERSION_NUM >= 0x072c00
> >> + curl_easy_setopt(result, CURLOPT_SSL_OPTIONS,
> >> CURLSSLOPT_NO_REVOKE);
> >> +#else
> >> + warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL
> >> options because\n"
> >> + "your curl version is too old (>= 7.44.0)");
> >
> > This message is confusing. If your curl is too old, shouldn't the ">=" be a
> > "<"?
>
> I do not think I saw any update to correct this, and worse yet I do
> not offhand recall if there was any other issue raised on the
> series.
Sorry, my bad. I dropped the ball. As you can see here:
https://github.com/gitgitgadget/git/pull/46
I have some updates that are already pushed, but I still wanted to really
think through your response here:
https://public-inbox.org/git/xmqq1s8oxbpc@gitster-ct.c.googlers.com/
and what I should do about it, before sending off v2. You can see that I
already updated the description in preparation for sending another
iteration.
I hope to get back to this tonight, for now I must scramble off to
non-work-related activities.
Ciao,
Dscho
> So assuming that this is the only remaining one, I'll squash the
> following to step 2/3 of this three-patch series and plan to merge
> it down to 'next' in the coming few days.
>
> I have a clean-up suggestion related to this but is orthogonal to
> this three-patch series (after the fix-up is applied, anyway), which
> I'll be sending out separately.
>
> Thanks.
>
> http.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/http.c b/http.c
> index 0ebf8f77a6..43e75ac583 100644
> --- a/http.c
> +++ b/http.c
> @@ -835,7 +835,7 @@ static CURL *get_curl_handle(void)
> curl_easy_setopt(result, CURLOPT_SSL_OPTIONS,
> CURLSSLOPT_NO_REVOKE);
> #else
> warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options
> because\n"
> - "your curl version is too old (>= 7.44.0)");
> + "your curl version is too old (< 7.44.0)");
> #endif
> }
>
> --
> 2.19.1-542-gc4df23f792
>
>
Re: [PATCH v3 2/3] khash: silence -Wunused-function in delta-islands from khash
On Thu, Oct 25, 2018 at 04:04:26AM -0700, Carlo Marcelo Arenas Belón wrote: > showing the following when compiled with latest clang (OpenBSD, Fedora > and macOS): s/^s/S/ This applies to your other commit messages as well. But more importantly, please be explicit about the compiler version that emits the warning, so others won't have to guess when stumbling upon this commit in a few months or years time. > delta-islands.c:23:1: warning: unused function 'kh_destroy_str' > [-Wunused-function] > delta-islands.c:23:1: warning: unused function 'kh_clear_str' > [-Wunused-function] > delta-islands.c:23:1: warning: unused function 'kh_del_str' > [-Wunused-function] > > Reported-by: René Scharfe > Suggested-by: Nguyễn Thái Ngọc Duy > Signed-off-by: Carlo Marcelo Arenas Belón > Signed-off-by: Junio C Hamano > --- > khash.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/khash.h b/khash.h > index d10caa0c35..532109c87f 100644 > --- a/khash.h > +++ b/khash.h > @@ -234,7 +234,7 @@ static const double __ac_HASH_UPPER = 0.77; > __KHASH_IMPL(name, SCOPE, khkey_t, khval_t, kh_is_map, __hash_func, > __hash_equal) > > #define KHASH_INIT(name, khkey_t, khval_t, kh_is_map, __hash_func, > __hash_equal) \ > - KHASH_INIT2(name, static inline, khkey_t, khval_t, kh_is_map, > __hash_func, __hash_equal) > + KHASH_INIT2(name, MAYBE_UNUSED static inline, khkey_t, khval_t, > kh_is_map, __hash_func, __hash_equal) > > /* Other convenient macros... */ > > -- > 2.19.1 >
'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress'
Hi, when branch 'ds/test-multi-pack-index' is merged with 'ab/commit-graph-progress', IOW 'master', 'next', or 'pu', 'GIT_TEST_MULTI_PACK_INDEX=1 ./t6500-gc.sh' fails with: expecting success: git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && test_must_be_empty stdout && test_line_count = 1 stderr && test_i18ngrep "Computing commit graph generation numbers" stderr + git -c gc.writeCommitGraph=true gc --no-quiet + test_must_be_empty stdout + test_path_is_file stdout + test -f stdout + test -s stdout + test_line_count = 1 stderr + test 3 != 3 + wc -l + test 16 = 1 + echo test_line_count: line count for stderr != 1 test_line_count: line count for stderr != 1 + cat stderr error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable Computing commit graph generation numbers: 25% (1/4) ^MComputing commit graph generation numbers: 50% (2/4) ^MComputing commit graph generation numbers: 75% (3/4) ^MComputing commit graph generation numbers: 100% (4/4) ^MComputing commit graph generation numbers: 100% (4/4), done. + return 1 error: last command exited with $?=1 not ok 9 - gc --no-quiet I suspect these "packfile index unavailable" errors are a Bad Thing, but I didn't follow the MIDX development closely enough to judge. Surprisingly (to me), 'git gc' didn't exit with error despite these errors. Anyway, this test seems to be too fragile, because that test_line_count = 1 stderr line will trigger, when anything else during 'git gc' prints something. And I find it quite strange that an option called '--no-quiet' only shows the commit-graph progress, but not the regular output of 'git gc'.
[PATCH v3 3/3] commit-slab: missing definitions and forward declarations (hdr-check)
struct commmit needs to be defined before commit-slab can generate working code, object_id should be at least known through a forward declaration Signed-off-by: Carlo Marcelo Arenas Belón --- commit-slab-impl.h | 2 ++ commit-slab.h | 2 ++ 2 files changed, 4 insertions(+) diff --git a/commit-slab-impl.h b/commit-slab-impl.h index e352c2f8c1..db7cf3f19b 100644 --- a/commit-slab-impl.h +++ b/commit-slab-impl.h @@ -1,6 +1,8 @@ #ifndef COMMIT_SLAB_IMPL_H #define COMMIT_SLAB_IMPL_H +#include "commit.h" + #define implement_static_commit_slab(slabname, elemtype) \ implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static) diff --git a/commit-slab.h b/commit-slab.h index 69bf0c807c..722252de61 100644 --- a/commit-slab.h +++ b/commit-slab.h @@ -1,6 +1,8 @@ #ifndef COMMIT_SLAB_H #define COMMIT_SLAB_H +struct object_id; + #include "commit-slab-decl.h" #include "commit-slab-impl.h" -- 2.19.1
[PATCH v3 1/3] commit-slab: move MAYBE_UNUSED into git-compat-util
after 36da893114 ("config.mak.dev: enable -Wunused-function", 2018-10-18)
it is expected to be used to prevent -Wunused-function warnings for code
that was macro generated
Signed-off-by: Carlo Marcelo Arenas Belón
Signed-off-by: Junio C Hamano
---
commit-slab-impl.h | 4 +---
git-compat-util.h | 2 ++
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/commit-slab-impl.h b/commit-slab-impl.h
index ac1e6d409a..e352c2f8c1 100644
--- a/commit-slab-impl.h
+++ b/commit-slab-impl.h
@@ -1,10 +1,8 @@
#ifndef COMMIT_SLAB_IMPL_H
#define COMMIT_SLAB_IMPL_H
-#define MAYBE_UNUSED __attribute__((__unused__))
-
#define implement_static_commit_slab(slabname, elemtype) \
- implement_commit_slab(slabname, elemtype, static MAYBE_UNUSED)
+ implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static)
#define implement_shared_commit_slab(slabname, elemtype) \
implement_commit_slab(slabname, elemtype, )
diff --git a/git-compat-util.h b/git-compat-util.h
index 48c955541e..8121b73b4a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -408,6 +408,8 @@ static inline char *git_find_last_dir_sep(const char *path)
#define LAST_ARG_MUST_BE_NULL
#endif
+#define MAYBE_UNUSED __attribute__((__unused__))
+
#include "compat/bswap.h"
#include "wildmatch.h"
--
2.19.1
[PATCH v3 0/3] delta-islands: avoid unused function messages
the macro generated code from delta-islands (using khash) triggers some unused function warnings in macOS, OpenBSD and some linux with a newer version of clang because of its use of static functions. Changes from v2: * Relay in the C code including git-compat-util as suggested by Jeff * Commit message cleanup * Make changes hdr-check clean Changes from v1: * Use MAYBE_UNUSED for all cases as suggested by Duy Carlo Marcelo Arenas Belón (3): commit-slab: move MAYBE_UNUSED into git-compat-util khash: silence -Wunused-function in delta-islands from khash commit-slab: missing definitions and forward declarations (hdr-check) commit-slab-impl.h | 4 ++-- commit-slab.h | 2 ++ git-compat-util.h | 2 ++ khash.h| 2 +- 4 files changed, 7 insertions(+), 3 deletions(-) -- 2.19.1
[PATCH v3 2/3] khash: silence -Wunused-function in delta-islands from khash
showing the following when compiled with latest clang (OpenBSD, Fedora and macOS): delta-islands.c:23:1: warning: unused function 'kh_destroy_str' [-Wunused-function] delta-islands.c:23:1: warning: unused function 'kh_clear_str' [-Wunused-function] delta-islands.c:23:1: warning: unused function 'kh_del_str' [-Wunused-function] Reported-by: René Scharfe Suggested-by: Nguyễn Thái Ngọc Duy Signed-off-by: Carlo Marcelo Arenas Belón Signed-off-by: Junio C Hamano --- khash.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/khash.h b/khash.h index d10caa0c35..532109c87f 100644 --- a/khash.h +++ b/khash.h @@ -234,7 +234,7 @@ static const double __ac_HASH_UPPER = 0.77; __KHASH_IMPL(name, SCOPE, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal) #define KHASH_INIT(name, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal) \ - KHASH_INIT2(name, static inline, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal) + KHASH_INIT2(name, MAYBE_UNUSED static inline, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal) /* Other convenient macros... */ -- 2.19.1
Re: [PATCH] Clear --exclude list after 'git rev-parse --all'
On Wed, Oct 24, 2018 at 11:49:06AM +0200, Andreas Gruenbacher wrote: > > That is why I asked what "problem" this patch fixes. Without > > answering that question, it is unclear if the patch is completing > > missing coverage for "--all", or it is cargo culting an useless > > clearing done for "--glob" and friends to code for "--all" that did > > not do the same useless clearing. > > This is how the --exclude option is described: > >--exclude= >Do not include refs matching that the next > --all, --branches, >--tags, --remotes, or --glob would otherwise consider. > Repetitions of this >option accumulate exclusion patterns up to the next --all, > --branches, --tags, >--remotes, or --glob option (other options or arguments do not > clear >accumulated patterns). > > I'm assuming that some thought has gone into making the options behave > in this particular way. The implementation in revision.c follows this > pattern, but the implementation in builtin/rev-parse.c only almost > does. Yeah. I think this is just a bug in 9dc01bf063 (rev-parse: introduce --exclude= to tame wildcards, 2013-11-01), in that it's handling of --all differed from e7b432c521 (revision: introduce --exclude= to tame wildcards, 2013-08-30). The clearing is very much intentional and documented, and in general rev-parse should behave the same as rev-list. An easy test is: $ git rev-list --no-walk --exclude='*' --all --all 3289ca716320457af5d2dd550b716282ad22da11 ...a bunch of other tip commits... $ git rev-parse --exclude='*' --all --all [no output, but it should print those same tip commits] -Peff
Re: git pull defaults for recursesubmodules
Tommi Vainikainen writes: > After reading SubmittingPatches I didn't find if I should now send a > fresh patch with > changes squashed together or new commits appended after first commit in that > patch. Patch is updated accordingly as fresh patch. (just on mechanics, not on the contents of your actual patch) You can and should treat any topic that is not yet in 'next' as if it did not exist. If you refined based on a v1 patch, pretend as if you were a perfect developer and you came up with that refined version without producing any problematic things that were pointed ont in your v1. Pretend mistakes in v1 never happened. Pretend that you are perfect! ;-) If you can limit the signs that an earlier rounds ever existed to (1) The In-reply-to: header of the message you send your updated version of the patch in, so that people can find the older version and its discussion thread, and (2) The cover letter that describes what you improved in the updated version relative to the last round, in addition to the overview of the series [note: this only exists for a larger patch series, and not usually done for a single patch] you achieved your goal.
Re: [PATCH v3] send-email: explicitly disable authentication
Joshua Watt writes:
> It can be necessary to disable SMTP authentication by a mechanism other
> than sendemail.smtpuser being undefined. For example, if the user has
> sendemail.smtpuser set globally but wants to disable authentication
> locally in one repository.
>
> --smtp-auth and sendemail.smtpauth now understand the value 'none' which
> means to disable authentication completely, even if an authentication
> user is specified.
>
> The value 'none' is lower case to avoid conflicts with any RFC 4422
> authentication mechanisms.
>
> The user may also specify the command line argument --no-smtp-auth as a
> shorthand for --smtp-auth=none
>
> Signed-off-by: Joshua Watt
> ---
Thanks. Let's queue this and merge it to 'next' soonish.
> Documentation/git-send-email.txt | 7 ++-
> git-send-email.perl | 8 ++--
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt
> b/Documentation/git-send-email.txt
> index 465a4ecbe..17993e3c9 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -190,7 +190,9 @@ $ git send-email --smtp-auth="PLAIN LOGIN GSSAPI" ...
> If at least one of the specified mechanisms matches the ones advertised by
> the
> SMTP server and if it is supported by the utilized SASL library, the
> mechanism
> is used for authentication. If neither 'sendemail.smtpAuth' nor `--smtp-auth`
> -is specified, all mechanisms supported by the SASL library can be used.
> +is specified, all mechanisms supported by the SASL library can be used. The
> +special value 'none' maybe specified to completely disable authentication
> +independently of `--smtp-user`
>
> --smtp-pass[=]::
> Password for SMTP-AUTH. The argument is optional: If no
> @@ -204,6 +206,9 @@ or on the command line. If a username has been specified
> (with
> specified (with `--smtp-pass` or `sendemail.smtpPass`), then
> a password is obtained using 'git-credential'.
>
> +--no-smtp-auth::
> + Disable SMTP authentication. Short hand for `--smtp-auth=none`
> +
> --smtp-server=::
> If set, specifies the outgoing SMTP server to use (e.g.
> `smtp.example.com` or a raw IP address). Alternatively it can
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2be5dac33..a70679484 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -82,8 +82,11 @@ sub usage {
> Pass an empty string to disable
> certificate
> verification.
> --smtp-domain * The domain name sent to HELO/EHLO
> handshake
> ---smtp-auth * Space-separated list of allowed AUTH
> mechanisms.
> +--smtp-auth * Space-separated list of allowed AUTH
> mechanisms, or
> + "none" to disable authentication.
> This setting forces to use one of the
> listed mechanisms.
> +--no-smtp-auth Disable SMTP authentication. Shorthand
> for
> + `--smtp-auth=none`
> --smtp-debug<0|1> * Disable, enable Net::SMTP debug.
>
> --batch-size * send max message per connection.
> @@ -341,6 +344,7 @@ sub signal_handler {
> "smtp-debug:i" => \$debug_net_smtp,
> "smtp-domain:s" => \$smtp_domain,
> "smtp-auth=s" => \$smtp_auth,
> + "no-smtp-auth" => sub {$smtp_auth = 'none'},
> "identity=s" => \$identity,
> "annotate!" => \$annotate,
> "no-annotate" => sub {$annotate = 0},
> @@ -1241,7 +1245,7 @@ sub smtp_host_string {
> # (smtp_user was not specified), and 0 otherwise.
>
> sub smtp_auth_maybe {
> - if (!defined $smtp_authuser || $auth) {
> + if (!defined $smtp_authuser || $auth || (defined $smtp_auth &&
> $smtp_auth eq "none")) {
> return 1;
> }
Re: [PATCH v3 7/7] revision.c: refactor basic topo-order logic
On Thu, Oct 11, 2018 at 12:21:44PM -0400, Derrick Stolee wrote: > > > 2. INDEGREE: using the indegree_queue priority queue (ordered > > > by maximizing the generation number), add one to the in- > > > degree of each parent for each commit that is walked. Since > > > we walk in order of decreasing generation number, we know > > > that discovering an in-degree value of 0 means the value for > > > that commit was not initialized, so should be initialized to > > > two. (Recall that in-degree value "1" is what we use to say a > > > commit is ready for output.) As we iterate the parents of a > > > commit during this walk, ensure the EXPLORE walk has walked > > > beyond their generation numbers. > > I wondered how this would work for INFINITY. We can't know the order of > > a bunch of INFINITY nodes at all, so we never know when their in-degree > > values are "done". But if I understand the EXPLORE walk, we'd basically > > walk all of INFINITY down to something with a real generation number. Is > > that right? > > > > But after that, I'm not totally clear on why we need this INDEGREE walk. > > The INDEGREE walk is an important element for Kahn's algorithm. The final > output order is dictated by peeling commits of "indegree zero" to ensure all > children are output before their parents. (Note: since we use literal 0 to > mean "uninitialized", we peel commits when the indegree slab has value 1.) > > This walk replaces the indegree logic from sort_in_topological_order(). That > method performs one walk that fills the indegree slab, then another walk > that peels the commits with indegree 0 and inserts them into a list. I guess my big question here was: if we have generation numbers, do we need Kahn's algorithm? That is, in a fully populated set of generation numbers (i.e., no INFINITY), we could always just pick a commit with the highest generation number to show. So if we EXPLORE down to a real generation number in phase 1, why do we need to care about INDEGREE anymore? Or am I wrong that we always have a real generation number (i.e., not INFINITY) after EXPLORE? (And if so, why is exploring to a real generation number a bad idea; presumably it's due to a worst-case that goes deeper than we'd otherwise need to here). > [...] Everything else you said here made perfect sense. -Peff
[PATCH v2] sequencer: cleanup for gcc warning in non developer mode
as shown by:
sequencer.c: In function ‘write_basic_state’:
sequencer.c:2392:37: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
write_file(rebase_path_verbose(), "");
where write_file will create an empty file if told to write an empty string
as can be inferred by the previous call
the somehow more convoluted syntax works around the issue by providing a non
empty format string and is already being used for the abort safety file since
1e41229d96 ("sequencer: make sequencer abort safer", 2016-12-07)
Signed-off-by: Carlo Marcelo Arenas Belón
---
Notes:
Changes in v2
- Avoid change of behaviour as suggested by Junio
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 8dd6db5a01..10f602c4d4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2335,7 +2335,7 @@ int write_basic_state(struct replay_opts *opts, const
char *head_name,
write_file(rebase_path_quiet(), "\n");
if (opts->verbose)
- write_file(rebase_path_verbose(), "");
+ write_file(rebase_path_verbose(), "%s", "");
if (opts->strategy)
write_file(rebase_path_strategy(), "%s\n", opts->strategy);
if (opts->xopts_nr > 0)
--
2.19.1
Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation
"brian m. carlson" writes: > Yeah, that behavior is quite old. I'm surprised that Linux ever did > that. > ... > I don't feel strongly either way. I feel confident the rest of Git > doesn't use that field, so I don't see any downsides to keeping it other > than the slight overhead of populating it. I just thought I'd ask in > case there was something important I was missing. OK, I'd consider that this part of the review settled for taking the patch as-is. Let's mark the topic for merging to 'next' soonish in the what's cooking report. Thanks.
Re: [PATCH v4 2/3] reset: add new reset.quiet config setting
Junio C Hamano writes: > To be honest, I find the second sentence in your rewrite even more > confusing. It reads as if `reset.quiet` configuration variable > can be used to restore the "show what is yet to be added" > behaviour, due to the parenthetical mention of the default behaviour > without any configuration. > > The command reports what is yet to be added to the index > after `reset` by default. It can be made to only report > errors with the `--quiet` option, or setting `reset.quiet` > configuration variable to `true` (the latter can be > overriden with `--no-quiet`). > > That may not be much better, though X-<. In any case, the comments are getting closer to the bikeshedding territory, that can be easily addressed incrementally. I am getting the impression that everbody agrees that the change is desirable, sufficiently documented and properly implemented. Shall we mark it for "Will merge to 'next'" in the what's cooking report and leave further refinements to incremental updates as needed?
Re: [PATCH v4 2/2] worktree: add per-worktree config files
Duy Nguyen writes: >> > +extensions.worktreeConfig:: >> > + If set, by default "git config" reads from both "config" and >> > + "config.worktree" file from GIT_DIR in that order. In >> > + multiple working directory mode, "config" file is shared while >> > + "config.worktree" is per-working directory (i.e., it's in >> > + GIT_COMMON_DIR/worktrees//config.worktree) >> > + >> >> This obviously conflicts with your 59-patch series, but more >> importantly >> >> - I notice that this is the only description of extensions.* key in >>the configuration files. Don't we have any other extension >>defined, and if so shouldn't we be describing them already (not >>as a part of this series, obviously)? > > Right. We have two extensions already but it's described in > technical/repository-format.txt. I'll move this extension there > because it's written "This document will serve as the master list for > extensions." in that document. > >> - If we are going to describe other extensions.* keys, do we want >>extensions-config.txt file to split this (and others) out and >>later rename it to config/extensions.txt? Or do we want to >>collect related things together by logically not by name and have >>this extension described in config/worktree.txt instead, perhaps >>separate from other extensions.* keys? > > I think we would go with config/extensions.txt because if grouping > logically, I'm not sure where extensions.preciousObjects and > extensions.partialClone would go. OK, that sounds sensible. Other than that, I am getting the feeling that everybody agrees that the problem this topic addresses is worth addressing, and the design and the implementation of the solution presented here is sensible. If so, let's move it forward to 'next' and plan to merge it down to 'master'. The "extensions.*" description can happen incrementally,. I'd think.
Re: [PATCH] sequencer: cleanup for gcc 8.2.1 warning
On Thu, Oct 25, 2018 at 1:08 AM Junio C Hamano wrote:
> For a single-use, not using the macro and just using "%s", "" should
> suffice.
OK, will send it as v2 then but would think it will be better if
applied as a "fixup" on top of the original branch:
34b47315d9 ("rebase -i: move rebase--helper modes to
rebase--interactive", 2018-09-27)
would be a good idea to include also enabling this warning in
developer mode so it doesn't sneak back?, presume as the last patch of
the refactor below?, FWIW this is the only case in current pu, and I
suspect the sooner we add it to next the less likely we will find
more.
> If we want to hide the "%s", "" trickery from distracting
> the readers (which is what you are trying to address with your
> touch_file() proposal, I think, and I also suspect that it may be a
> legitimate one), I tend to think that it may be a better solution to
> introduce the EMPTY_PRINTF_ARG macro and use it everywhere, in
> builtin/commit.c, builtin/difftool.c and wt-status.c and not not
> just here.
will work on this in a different feature branch, but I had to admit I
don't like it for status_printf case where it was IMHO a hack to get
the new lines to begin with.
I would think it would make more sense to call a function that says
"write_ttycolor_ln" instead for those cases.
Carlo
Re: [PATCH 18/19] submodule: use submodule repos for object lookup
On Tue, Oct 16, 2018 at 04:35:49PM -0700, Stefan Beller wrote: > This converts the 'show_submodule_header' function to use > the repository API properly, such that the submodule objects > are not added to the main object store. This patch breaks the test suite with 'GIT_TEST_COMMIT_GRAPH=1', in particular 't4041-diff-submodule-option.sh' fails with: expecting success: git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head2..$head3 (rewind): < Add foo3 ($added foo3) < Add foo2 ($added foo2) EOF test_cmp expected actual + git diff-index -p --submodule=log HEAD + cat + test_cmp expected actual + diff -u expected actual --- expected2018-10-25 09:10:00.541610016 + +++ actual 2018-10-25 09:10:00.537609885 + @@ -1,3 +1,5 @@ -Submodule sm1 30b9670..dafb207 (rewind): +Submodule sm1 30b9670...dafb207: < Add foo3 (hinzugefügt foo3) < Add foo2 (hinzugefügt foo2) + > Add foo1 (hinzugefügt foo1) + < Add foo1 (hinzugefügt foo1) error: last command exited with $?=1 not ok 9 - modified submodule(backward) and 't4060-diff-submodule-option-diff-format.sh' fails with: expecting success: git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head2..$head3 (rewind): diff --git a/sm1/foo2 b/sm1/foo2 deleted file mode 100644 index 54b060e..000 --- a/sm1/foo2 +++ /dev/null @@ -1 +0,0 @@ -foo2 diff --git a/sm1/foo3 b/sm1/foo3 deleted file mode 100644 index c1ec6c6..000 --- a/sm1/foo3 +++ /dev/null @@ -1 +0,0 @@ -foo3 EOF test_cmp expected actual + git diff-index -p --submodule=diff HEAD + cat + test_cmp expected actual + diff -u expected actual --- expected2018-10-25 09:10:38.854868800 + +++ actual 2018-10-25 09:10:38.854868800 + @@ -1,4 +1,4 @@ -Submodule sm1 30b9670..dafb207 (rewind): +Submodule sm1 30b9670...dafb207: diff --git a/sm1/foo2 b/sm1/foo2 deleted file mode 100644 index 54b060e..000 error: last command exited with $?=1 not ok 10 - modified submodule(backward)
Re: [PATCH] fetch-pack: be more precise in parsing v2 response
Junio C Hamano writes: > Jonathan Tan writes: > >> +GIT_TRACE_PACKET="$(pwd)/log" test_must_fail git -C http_child \ >> +-c protocol.version=2 \ >> +fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err && > > Because test_must_fail is a shell function, the above is not a > correct way to say "I want GIT_TRACE_PACKET exported only while this > thing runs". > > I'll squash the following in. > > t/t5702-protocol-v2.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh > index 51009ca391..d58fbfa9e5 100755 > --- a/t/t5702-protocol-v2.sh > +++ b/t/t5702-protocol-v2.sh > @@ -555,7 +555,7 @@ test_expect_success 'when server does not send "ready", > expect FLUSH' ' > printf "/acknowledgments/,$ s//0001/" \ > >"$HTTPD_ROOT_PATH/one-time-sed" && > > - GIT_TRACE_PACKET="$(pwd)/log" test_must_fail git -C http_child \ > + test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" git -C http_child \ > -c protocol.version=2 \ > fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err && > grep "fetch< acknowledgments" log && I know it only has been a few days, but is there any other issue in the patch, anybody? Otherwise, I am wondering if we can move this forwared after squashing the above fix in. Thanks.
