Re: [PATCH] howto/using-merge-subtree: mention --allow-unrelated-histories
Uwe Kleine-König writes:
> Without passing --allow-unrelated-histories the command sequence
> fails as intended since commit e379fdf34fee ("merge: refuse to create
> too cool a merge by default"). To setup a subtree merging unrelated
> histories is normal, so add the option to the howto document.
>
Thanks. We should have been more careful when we tightened "git
merge".
Will apply.
> Signed-off-by: Uwe Kleine-König
> ---
> Documentation/howto/using-merge-subtree.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/howto/using-merge-subtree.txt
> b/Documentation/howto/using-merge-subtree.txt
> index 1ae8d1214ec0..a499a94ac228 100644
> --- a/Documentation/howto/using-merge-subtree.txt
> +++ b/Documentation/howto/using-merge-subtree.txt
> @@ -33,7 +33,7 @@ Here is the command sequence you need:
>
>
> $ git remote add -f Bproject /path/to/B <1>
> -$ git merge -s ours --no-commit Bproject/master <2>
> +$ git merge -s ours --no-commit --allow-unrelated-histories Bproject/master
> <2>
> $ git read-tree --prefix=dir-B/ -u Bproject/master <3>
> $ git commit -m "Merge B project as our subdirectory" <4>
Re: the opposite of .gitignore, whitelist
"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?
Re: [PATCH] sequencer: cleanup for gcc 8.2.1 warning
Carlo Marcelo Arenas Belón writes: > 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(), ""); The change may squelch the above warning, but doesn't it change the behaviour? You need to explain what the changed behaviour is and why it is OK to change that behaviour in this space, in addition to show what warning you are working around. I _think_ the intent of this code is to create an empty file, because that is what the original version of "git rebase" did. write_file() does use strbuf_complete_line() to avoid creating a file with incomplete last line, but it does allow us to create an empty file. By adding a LF, the created file is no longer an empty one. Does it matter? IOW, does it cause a regression if we start adding an LF to the file? By reading master:git-rebase.sh::read_basic_state(), I _think_ it is safe to do so, as the side that consumes this $state_dir/verbose only checks file's existence, and does not care what it contains, even if somehow the scripted version of "git rebase" ends up reading the state file created by this newer version of "git rebase" in C. Also next:sequencer.c::read_populate_opts() only checks for file's existence, so the newer version of "git rebase" is also safe. So as an immediate workaround for 'next', this patch happens to be OK, but that is only true because the consumer happens to ignore the distinction between an empty file and a file with a single LF in it. I'd have to say that the ability to create an empty file is more important in the longer term. Can't the workaround be done to write_file() instead? I actually do not mind if the solution were to introduce a newhelper "write_empty_file()", but the way it is written in the code before this patch, i.e. write_file(FILENAME, "") is so obvious a way to create an empty file, so if we do not have to resort to such a hackery to special case an empty file, that would be preferrable. Thanks. > > Signed-off-by: Carlo Marcelo Arenas Belón > --- > sequencer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sequencer.c b/sequencer.c > index 8dd6db5a01..358e83bf6b 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(), "\n"); > if (opts->strategy) > write_file(rebase_path_strategy(), "%s\n", opts->strategy); > if (opts->xopts_nr > 0)
Re: [PATCH v4 2/3] reset: add new reset.quiet config setting
Ramsay Jones writes: >> diff --git a/Documentation/config.txt b/Documentation/config.txt >> index f6f4c21a54..a2d1b8b116 100644 >> --- a/Documentation/config.txt >> +++ b/Documentation/config.txt >> @@ -2728,6 +2728,9 @@ rerere.enabled:: >> `$GIT_DIR`, e.g. if "rerere" was previously used in the >> repository. >> >> +reset.quiet:: >> +When set to true, 'git reset' will default to the '--quiet' option. > > Mention that this 'Defaults to false'? Perhaps. >> -q:: >> --quiet:: >> -Be quiet, only report errors. >> +--no-quiet:: >> +Be quiet, only report errors. The default behavior is set by the >> +`reset.quiet` config option. `--quiet` and `--no-quiet` will >> +override the default behavior. > > Better than last time, but how about something like: > > -q:: > --quiet:: > --no-quiet:: > Be quiet, only report errors. The default behaviour of the > command, which is to not be quiet, can be specified by the > `reset.quiet` configuration variable. The `--quiet` and > `--no-quiet` options can be used to override any configured > default. > > Hmm, I am not sure that is any better! :-D 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-<.
Re: [PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name
Eric Sunshine writes: >> +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_must_fail git config user.email && > > Instead of simply asserting that 'user.email' is not set here, you > could instead proactively ensure that it is not set. That is, instead > of the test_must_fail(), do this: > > test_unconfig user.email && > test_unconfig user.name && Yes, it would be more in line with what is done to the environment variables and to other configuration variables in the same block. Not that I think that this inconsistency is end of the world ;-) Thanks. >> +echo changed >1.t && >> +git stash >> +' >> + >> test_done >> -- >> 2.19.1.windows.1 >>
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
Slavica writes: > On 23-Oct-18 8:52 PM, Christian Couder wrote: >> On Tue, Oct 23, 2018 at 6:35 PM Slavica wrote: >>> This is part of enhancement request that ask for `git stash` to work even >>> if `user.name` is not configured. >>> The issue is discussed here: >>> https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u. >> We prefer commit messages that contain as much as possible all the >> information necessary to understand the patch without links to other >> places. >> >> It seems that only this email from you reached me. Did you send other >> emails for patches 2/3 and 3/3? >> >> [...] > > Okay, I will change that. This is my first patch and I am still adapting. > > Emails for patches 2/3 and 3/3 because aren't there because I am still > preparing them. > > (I didn't know if I had 3 patches in plan that they should be sent at > almost the same time.) It is more efficient for everybody involved. - You may discover that 1/3 you just (thought) finished was not sufficient while working on 2/3 and 3/3, and by the time you are pretty close to finishing 2/3 and 3/3, you may want to update 1/3 in a big way. Sending a premature version and having others to review is wasting everbody's time. - Your 1/3 might become perfect alone with help from others' reviews and your updates, but after that everybody may forget about it when you are ready to send out 2/3 and 3/3; if these three are truly related patches in a single topic, you would want to have what 1/3 did fresh in your reviewers' minds. You'd have to find the old message of 1/3 and make 2/3 and 3/3 responses to it to keep them properly threaded (which may take your time), and reviewers need to refresh their memory by going back to 1/3 before reviewing 2/3 and 3/3 One thing I learned twice while working in this project is that open source development is not a race to produce and show your product as quickly as possible. When I was an individual contributor, the project was young and there were many people with good and competing ideas working to achieve more-or-less the same goal. It felt like a competition to get *MY* version of the vision, design and implementation over others' adopted and one way to stay in the competition was to send things as quickly as possible. I didn't know better, and I think I ended up wasting many people's time that way. That changed when I became the maintainer, as (1) I no longer had to race with anybody ;-), and (2) I introduced the 'pu' (proposed update) system so that anything that was queued early can be discarded and replaced when a better thing come within a reasonable timeframe. And then I re-learned the same "this is not a race" lesson a couple of years ago, when I started working in a timezone several hours away from the most active participants for a few months at a time. I do not have to respond to a message I see on the list immediately, as it is too late to catch the sender who is already in bed ;-) So take your time and make sure what you are sending out can be reviewed the most efficiently. Completing 2/3 and 3/3 before sending 1/3 out to avoid having to redo 1/3 and avoid having reviewers to spend their time piecemeal is one thing. Making sure that the patch does not have style issues that distract reviewers' attention is another. Sitting on what you think you have completed for a few days allows you to review your product with fresh eyes before sending them out, which is another benefit of trying not to rush.
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
Johannes Schindelin writes: >> HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do >> so to avoid getting affected by the real $HOME/.gitconfig of the >> user who happens to be running the test suite. > > My bad. I should have checked. I was under the impression that we set > `HOME` to the `t/` directory and initialized it. But you are right, of > course, and that subshell as well as the override of `HOME` are absolutely > unnecessary. I was afraid that I may be missing some future plans to update $TRASH_DIRECTORY/.gitconfig with "git config --global user.name Foo" etc. in an earlier part of the test script, which would have made the subshell and moving HOME elsewhere perfectly good ways to future proof the new test being added (in which case, in-code comment to say that near the assignment to HOME would have been a good improvement). Not that having them breaks the logic, but they distract the readers by making them wonder what is going on, so I think we can do without the subshell and assignment to HOME. Thanks.
Re: [PATCH v2 0/2] Work around case-insensitivity issues with cwd on Windows
Johannes Schindelin writes: > On Wed, 24 Oct 2018, Junio C Hamano wrote: > >> "Johannes Schindelin via GitGitGadget" >> writes: >> >> > Changes since v1: >> > >> > * Fixed a grammar mistake in the second commit message. >> >> Thanks. I think this matches what I earlier queued on 'pu' with >> Stephen's typofix squashed in, so we are good already. > > Perfect. Sorry that I forgot to check. Thanks for double-checking, and I do appreciate the final version, even if it matches the version I would have in 'pu' if I perfectly followed all what was said in the review thread, as I am not always perfect.
Re: Recommended configurations (was Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting)
Jeff King writes: > I do hope that some options will just be no-brainers to enable always, > though (e.g., I think in the long run commit-graph should just default > to "on"; it's cheap to keep up to date and helps proportionally to the > repo size). Same here. We should strive to make any feature to optimize for repositories with a particular trait not to hurt too much to repositories without that trait, so that we can start such a feature as opt-in but later can make it the default for everybody. Sometimes it may not be possible, but my gut feeling is that features aiming for optimizing big repositories should fundamentally need only very small overhead when enabled in a small repository. So I view them not as a set of million "if your repository matches this criterion, turn it on" knobs. Rather, they are "we haven't tested it fully, but you can opt into the experiment a new way to do the same operation, which is designed to optimize for repositories with this trait. Enabling it even when your repository does not have that trait and reporting regression is also very welcome, as it is a good indication that the new way has rough edges at its corners". Thanks.
Re: [PATCH] Poison gettext with the Ook language
Duy Nguyen writes:
> The person who writes
>
> printf(_("%s"), getenv("foo"));
>
> may not go through the same thought process as with complexFunction().
> If _() calls getenv(), because you the order of parameter evaluation
> is unspecified, you cannot be sure if getenv("foo") will be called
> before or after the one inside _(). One of them may screw up the
> other.
Yup, sometimes we've been sloppy but we should strive to mimick
efforts like f4ef5173 ("determine_author_info(): copy getenv
output", 2014-08-27).
[PATCH] http: give curl version warnings consistently
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 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 2/3] http: add support for disabling SSL revocation checks in cURL
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.
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 1/2] t5410: use longer path for sample script
Johannes Schindelin writes: > On Wed, 24 Oct 2018, Jeff King wrote: > >> t5410 creates a sample script "alternate-refs", and sets >> core.alternateRefsCommand to just "alternate-refs". That >> shouldn't work, as "." is not in our $PATH, and so we should >> not find it. >> >> However, due to a bug in run-command.c, we sometimes find it >> anyway! Even more confusing, this bug is only in the >> fork-based version of run-command. So the test passes on >> Linux (etc), but fails on Windows. >> >> In preparation for fixing the run-command bug, let's use a >> more complete path here. >> >> Reported-by: Johannes Schindelin >> Signed-off-by: Jeff King >> --- > > Thank you for the fix! I can confirm that the patch works, and the commit > message is stellar, as per usual for your contributions. > > BTW since this breaks every single one of our Continuous Builds on > Windows, I would be very much in favor of fast-tracking this to `master`. > > Thanks, > Dscho I should note to the public that this one, and the companion patch 2/2, owe greatly to you and Peff's efforts. Thanks, both. > >> t/t5410-receive-pack-alternates.sh | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/t/t5410-receive-pack-alternates.sh >> b/t/t5410-receive-pack-alternates.sh >> index 457c20c2a5..f00d0da860 100755 >> --- a/t/t5410-receive-pack-alternates.sh >> +++ b/t/t5410-receive-pack-alternates.sh >> @@ -23,7 +23,7 @@ test_expect_success 'with core.alternateRefsCommand' ' >> --format="%(objectname)" \ >> refs/heads/public/ >> EOF >> -test_config -C fork core.alternateRefsCommand alternate-refs && >> +test_config -C fork core.alternateRefsCommand ./alternate-refs && >> git rev-parse public/branch >expect && >> printf "" | git receive-pack fork >actual && >> extract_haves actual.haves && >> -- >> 2.19.1.1094.gd480080bf6 >> >>
Re: [PATCH v3 2/3] reset: add new reset.quiet config setting
Duy Nguyen writes: > OK. Just to be sure we're on the same page. Am I waiting for all > config changes to land in 'master', or do I rebase my series on > 'next'? I usually base on 'master' but the mention of 'next' here > confuses me a bit. I was hoping that you can do something like: $ git fetch https://github.com/gitster/git \ --refmap=refs/heads/*:refs/remotes/broken-out/* \ bp/reset-quiet master $ git checkout broken-out/master^0 $ git merge broekn-out/bp/reset-quiet $ git rebase HEAD np/config-split once it is clear to everybody that Ben's reset series is ready to be merged to 'next' (or it actually hits 'next').
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
Jeff King writes: > but then you lose the default handling. I think if we added a new > option, it would either be: > > # interpret a value directly; use default on empty, I guess? > git config --default=false --type=bool --interpret-value "$GIT_WHATEVER_ENV" > > or > > # less flexible, but the --default semantics are more obvious > git config --default=false --type=bool --get-env GIT_WHATEVER_ENV 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).
Re: [PATCH v2 0/2] Work around case-insensitivity issues with cwd on Windows
"Johannes Schindelin via GitGitGadget" writes: > Changes since v1: > > * Fixed a grammar mistake in the second commit message. Thanks. I think this matches what I earlier queued on 'pu' with Stephen's typofix squashed in, so we are good already.
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
Johannes Schindelin writes: > Hi Junio, > > On Wed, 24 Oct 2018, Junio C Hamano wrote: > >> Slavica writes: >> >> > +test_expect_failure 'stash with HOME as non-existing directory' ' >> > +test_commit 1 && >> > +test_config user.useconfigonly true && >> > +test_config stash.usebuiltin true && >> > +( >> > +HOME=$(pwd)/none && >> > +export HOME && >> >> What is the reason why this test needs to move HOME away from >> TRASH_DIRECTORY (set in t/test-lib.sh)? > > This is to make sure that no user.name nor user.email is configured. That > was my idea, hence I answer your question. HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do so to avoid getting affected by the real $HOME/.gitconfig of the user who happens to be running the test suite. With that in place for ages, this test still moves HOME away from TRASH_DIRECTORY, and that is totally unnecessary if it is only done to avoid getting affected by the real $HOME/.gitconfig of the user. After all, this single test is not unique in its need to avoid reading from user's $HOME/.gitconfig---so I expected there may be other reasons. That is why I asked, and your response is not quite answering that question.
Re: [PATCH] Clear --exclude list after 'git rev-parse --all'
Andreas Gruenbacher writes: >> All other glob options do show_reference with for_each_ref_in() and >> then calls clear_ref_exclusion(), and logically the patch makes >> sense. >> >> What is the "problem" this patch fixes, though? Is it easy to add a >> new test to t/6018-rev-list-glob.sh to demonstrate that "--glob" (or >> whatever that clears exclusion list without this patch) works >> correctly but "--all" misbehaves without this change? > > The test suite doesn't cover clearing the exclusion list for any of > those rev-parse options and I also didn't write such a test case. I > ran into this inconsistency during code review. 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. IOW, there are two ways to address the "inconsistency", and the proposed log message (nor your answer above) does not make a convincing argument why adding the same code to the "--all" side is the right way to achieve consistency---rather than removing the call to clear from the existing ones. Thanks.
Re: [PATCH v3 2/3] reset: add new reset.quiet config setting
Junio C Hamano writes: >> Seems a bit overkill to pull a line of documentation into a separate >> file and replace it with a line of 'import' logic. Perhaps if/when >> there is more documentation to pull out that would make more sense. > > This change (ehh, rather, perhaps nd/config-split topic) came at an > unfortunate moment. Until I actually did one integration cycle to > rebuild 'pu' and merge this patch and the other topic, I had exactly > the same reaction as yours above to Duy's comment. But seeing the > tree at the tip of 'pu' today, I do think the end result with a > single liner file that has configuration for the "reset" command > that is included in config.txt would make sense, and I also think > you would agree with it if you see the same tree. > > How we should get there is a different story. I think Duy's series > needs at least another update to move the split pieces into its own > subdirectory of Documentation/, and it is not all that urgent, while > this three-patch series (with the advice.* bit added) for "reset" is > pretty much ready to go 'next', so my gut feeling is that it is best > to keep the description here, and to ask Duy to base the updated > version of config-split topic on top. I'll take the "it is not all that urgent" bit (and only that bit) back, even though the conclusion would be the same. It is quite painful having to keep this topic while a few topics that touch the huge Documentation/config.txt is in flight. The monolithic file is large enough that it does not cause much pain while many topics are in flight, but the single step of spliting it into million pieces done by nd/config-split topic is a pain to merge. Anybody interested may fetch, and try $ git checkout 5c2d198e8e $ git merge b2358ceaca and imagine that you'd have to redo that every time somebody adds or touches up a byte in the description of an individual entry in the Documentation/config.txt file. It is not pretty until we finish the split.
Re: Bug with "git mv" and submodules, and with "git submodule add something --force"
Stefan Beller writes: >> You would want to be able to remove a submodule and replace it with >> a directory, but you can probably do it in two steps, i.e. >> >> git reset --hard >> git rm --cached sha1collisiondetection >> echo Now a regular dir >sha1collisiondetection/READ.ME >> find sha1collisiondetection ! -type d -print0 | >> git update-index --add --stdin -z > > "Ignoring path sha1collisiondetection/.git" > > Nice! There actually is another possible outcome that anybody following along must be aware of and be careful about: not even .git directory exist there, i.e. it is possible that the submodule has never been init'ed. So, it is not that nice X-<.
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
Ævar Arnfjörð Bjarmason writes:
> Notes on the implementation:
>
> * The only reason we need a new "git-sh-i18n--helper" and the
>corresponding "test-tool gettext-poison" is to expose
>git_env_bool() to shellscripts, since git-sh-i18n and friends need
>to inspect the $GIT_TEST_GETTEXT_POISON variable.
>
>We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the
>test suite, and this code can go away for non-test code once the
>last i18n-using shellscript is rewritten in C.
Makes me wonder if we want to teach "git config" a new "--env-bool"
option that can be used by third-party scripts. Or would it be just
the matter of writing
git config --default=false --type=bool "$GIT_WHATEVER_ENV"
in these third-party scripts and we do not need to add such a thing?
> * The reason for not doing:
>
>test_lazy_prereq GETTEXT_POISON 'test-tool gettext-poison'
>test_lazy_prereq C_LOCALE_OUTPUT '! test-tool gettext-poison'
>
>In test-lib.sh is because there's some interpolation problem with
>that syntax which makes t6040-tracking-info.sh fail. Hence using
>the simpler test_set_prereq.
s/In/in/, as you haven't finished the sentence yet. But more
importantly, what is this "some interpolation problem"? Are you
saying that test_lazy_prereq implementation is somehow broken and
cannot take certain strings? If so, perhaps we want to fix that,
and people other than you can help to do so, once you let them know
what the problem is.
> See also
> https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/ for
> more discussion.
"See also [*1*] for more discussion" as you've already have
reference below.
>
> 1. https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
>
> Here's a polished version of that. I think it makes sense to queue
> this up before any other refactoring of GETTEXT_POISON, and some patch
> to unconditionally preserve format specifiers as I suggested upthread
> could go on top of this.
> ...
> +int cmd_sh_i18n__helper(int argc, const char **argv, const char *prefix)
> +{
> + int poison = -1;
> + struct option options[] = {
> + OPT_BOOL(0, "git-test-gettext-poison", ,
> + N_("is GIT_TEST_GETTEXT_POISON in effect?")),
> + OPT_END()
> + };
> +
> + argc = parse_options(argc, argv, NULL, options,
> + builtin_sh_i18n_helper_usage,
> PARSE_OPT_KEEP_ARGV0);
> +
> + if (poison != -1)
> + return !git_env_bool("GIT_TEST_GETTEXT_POISON", 0);
Hmm? If "--[no-]git-test-gettext-poison" is given, poison is either
0 or 1, and we return the value we read from the environment? What
convoluted way to implement the option is that, or is there anything
subtle that I am not getting?
If the "default" parameter to git_env_bool() were poison, and then
the option was renamed to "--get-git-text-gettext-poison", then I
sort of understand the code, though (it's like "git config" with
"--default" option).
But if there is nothing subtle, it may make sense to implement this
as an opt-cmdmode instead.
> diff --git a/po/README b/po/README
> index fef4c0f0b5..dba46c4a40 100644
> --- a/po/README
> +++ b/po/README
> @@ -289,16 +289,11 @@ something in the test suite might still depend on the
> US English
> version of the strings, e.g. to grep some error message or other
> output.
>
> -To smoke out issues like these Git can be compiled with gettext poison
> -support, at the top-level:
> +To smoke out issues like these Git tested with a translation mode that
> +emits gibberish on every call to gettext. To use it run the test suite
> +with it, e.g.:
s/these Git tested/these, Git can be tested/; even though the comma
is not a new issue you introduced.
Re: [PATCH] range-diff: allow to diff files regardless submodule
Junio C Hamano writes: > Lucas De Marchi writes: > >> Your reply arrived just a little after I sent the v2, so I thought it >> was just the race and you would end up seeing the unread email in the >> same thread. Sorry for not including the msg id: >> 20181011081750.24240-1-lucas.demar...@intel.com > > OK, then I am not surprised that I do not recall seeing such an old > message ;-) Let me take a look. Heh, it turns out that that is the version that has been queued in 'next' for about a week already. One issue that was pointed out in v1 was that the log message was unclear; it seems v2 didn't address that at all, though. Thanks.
Re: [PATCH] range-diff: allow to diff files regardless submodule
Lucas De Marchi writes: > Your reply arrived just a little after I sent the v2, so I thought it > was just the race and you would end up seeing the unread email in the > same thread. Sorry for not including the msg id: > 20181011081750.24240-1-lucas.demar...@intel.com OK, then I am not surprised that I do not recall seeing such an old message ;-) Let me take a look. Thanks.
Re: [PATCH 1/2] mergetool: Accept -g/--[no-]gui as arguments
Denton Liu writes:
> Subject: Re: [PATCH 1/2] mergetool: Accept -g/--[no-]gui as arguments
Other people may point it out, but s/Accept/accept/.
> In line with how difftool accepts a -g/--[no-]gui option, make mergetool
> accept the same option in order to use the `merge.guitool` variable to
> find the default mergetool instead of `merge.tool`.
> ...
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 9a8b97a2a..e317ae003 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -350,17 +350,23 @@ guess_merge_tool () {
> }
>
> get_configured_merge_tool () {
> - # Diff mode first tries diff.tool and falls back to merge.tool.
> - # Merge mode only checks merge.tool
> + # If first argument is true, find the guitool instead
> + if [ "$1" = true ]
Don't use [] in our shell script (see Documentation/CodingGuidelines);
say
if "$1" = true
instead.
> + then
> + gui_prefix=gui
> + fi
> +
> + # Diff mode first tries diff.(gui)tool and falls back to
> merge.(gui)tool.
> + # Merge mode only checks merge.(gui)tool
> if diff_mode
> then
> - merge_tool=$(git config diff.tool || git config merge.tool)
> + merge_tool=$(git config diff.${gui_prefix}tool || git config
> merge.${gui_prefix}tool)
> else
> - merge_tool=$(git config merge.tool)
> + merge_tool=$(git config merge.${gui_prefix}tool)
> fi
In mode_ok shell function, we seem to be prepared to deal with a
case where neither diff_mode nor merge_mode is true. Should this
codepath also be prepared to?
This is not a comment to point out an issue with this patch. It is
a genuine question on the code after (or before for that matter)
this patch is applied.
Thanks.
Re: [PATCH] revision.c: drop missing objects from cmdline
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). > 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?
Re: [PATCH] Clear --exclude list after 'git rev-parse --all'
Andreas Gruenbacher writes:
> Commit [1] added the --exclude option to revision.c. The --all,
> --branches, --tags, --remotes, and --glob options clear the exclude
> list. Shortly therafter, commit [2] added the same to 'git rev-parse',
> but without clearing the exclude list for the --all option. Fix that.
>
> [1] e7b432c52 ("revision: introduce --exclude= to tame wildcards",
> 2013-08-30)
> [2] 9dc01bf06 ("rev-parse: introduce --exclude= to tame wildcards",
> 2013-11-01)
>
> Signed-off-by: Andreas Gruenbacher
> ---
> builtin/rev-parse.c | 1 +
> 1 file changed, 1 insertion(+)
All other glob options do show_reference with for_each_ref_in() and
then calls clear_ref_exclusion(), and logically the patch makes
sense.
What is the "problem" this patch fixes, though? Is it easy to add a
new test to t/6018-rev-list-glob.sh to demonstrate that "--glob" (or
whatever that clears exclusion list without this patch) works
correctly but "--all" misbehaves without this change?
Thanks.
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 0f09bbbf6..c71e3b104 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -764,6 +764,7 @@ int cmd_rev_parse(int argc, const char **argv, const char
> *prefix)
> }
> if (!strcmp(arg, "--all")) {
> for_each_ref(show_reference, NULL);
> + clear_ref_exclusion(_excludes);
> continue;
> }
> if (skip_prefix(arg, "--disambiguate=", )) {
Re: [PATCH v3 3/3] repack -ad: prune the list of shallow commits
"Johannes Schindelin via GitGitGadget" writes: > diff --git a/builtin/repack.c b/builtin/repack.c > index c6a7943d5..9217fc832 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -549,6 +549,12 @@ int cmd_repack(int argc, const char **argv, const char > *prefix) > if (!po_args.quiet && isatty(2)) > opts |= PRUNE_PACKED_VERBOSE; > prune_packed_objects(opts); > + > + if (!keep_unreachable && > + (!(pack_everything & LOOSEN_UNREACHABLE) || > + unpack_unreachable) && > + is_repository_shallow(the_repository)) > + prune_shallow(0, 1); The logic looks correct (and the reasoning behind it, which was explained in the log message, was sound). prune_shallow(0, 1) however is not easily understandable. There are only two callsites of this function after these three patches, and the areas of the code that have these calls are in relatively quiescent parts in the whole tree, so it shouldn't be too distracting to do an update to make the function take a flag word, so that we can make callsites more immediately obvious, i.e. prune_shallow(PRUNE_SHALLOW_QUICK) It of course can be left as a low-hanging fruit loose-end. > diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh > index 2d0031703..777c9d1dc 100755 > --- a/t/t5537-fetch-shallow.sh > +++ b/t/t5537-fetch-shallow.sh > @@ -186,7 +186,7 @@ EOF > test_cmp expect actual > ' > > -test_expect_failure '.git/shallow is edited by repack' ' > +test_expect_success '.git/shallow is edited by repack' ' > git init shallow-server && > test_commit -C shallow-server A && > test_commit -C shallow-server B &&
Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries
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.
"Johannes Schindelin via GitGitGadget"
writes:
> From: Johannes Schindelin
>
> The `prune_shallow()` function wants a full reachability check to be
> completed before it goes to work, to ensure that all unreachable entries
> are removed from the shallow file.
>
> However, in the upcoming patch we do not even want to go that far. We
> really only need to remove entries corresponding to pruned commits, i.e.
> to commits that no longer exist.
>
> Let's support that use case.
>
> Signed-off-by: Johannes Schindelin
> ---
> builtin/prune.c | 2 +-
> commit.h| 2 +-
> shallow.c | 22 +-
> 3 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/prune.c b/builtin/prune.c
> index 41230f821..6d6ab6cf1 100644
> --- a/builtin/prune.c
> +++ b/builtin/prune.c
> @@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char
> *prefix)
> free(s);
>
> if (is_repository_shallow(the_repository))
> - prune_shallow(show_only);
> + prune_shallow(show_only, 0);
>
> return 0;
> }
> diff --git a/commit.h b/commit.h
> index 1d260d62f..ff34447ab 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -249,7 +249,7 @@ extern void assign_shallow_commits_to_refs(struct
> shallow_info *info,
> uint32_t **used,
> int *ref_status);
> extern int delayed_reachability_test(struct shallow_info *si, int c);
> -extern void prune_shallow(int show_only);
> +extern void prune_shallow(int show_only, int quick_prune);
> extern struct trace_key trace_shallow;
>
> extern int interactive_add(int argc, const char **argv, const char *prefix,
> int patch);
> diff --git a/shallow.c b/shallow.c
> index 732e18d54..0a2671bc2 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct
> repository *r)
>
> #define SEEN_ONLY 1
> #define VERBOSE 2
> +#define QUICK_PRUNE 4
>
> struct write_shallow_data {
> struct strbuf *out;
> @@ -261,7 +262,11 @@ static int write_one_shallow(const struct commit_graft
> *graft, void *cb_data)
> const char *hex = oid_to_hex(>oid);
> if (graft->nr_parent != -1)
> return 0;
> - if (data->flags & SEEN_ONLY) {
> + if (data->flags & QUICK_PRUNE) {
> + struct commit *c = lookup_commit(the_repository, >oid);
> + if (!c || parse_commit(c))
> + return 0;
> + } else if (data->flags & SEEN_ONLY) {
> struct commit *c = lookup_commit(the_repository, >oid);
> if (!c || !(c->object.flags & SEEN)) {
> if (data->flags & VERBOSE)
> @@ -371,16 +376,23 @@ void advertise_shallow_grafts(int fd)
>
> /*
> * mark_reachable_objects() should have been run prior to this and all
> - * reachable commits marked as "SEEN".
> + * reachable commits marked as "SEEN", except when quick_prune is non-zero,
> + * in which case lines are excised from the shallow file if they refer to
> + * commits that do not exist (any longer).
> */
> -void prune_shallow(int show_only)
> +void prune_shallow(int show_only, int quick_prune)
> {
> struct lock_file shallow_lock = LOCK_INIT;
> struct strbuf sb = STRBUF_INIT;
> + unsigned flags = SEEN_ONLY;
> int fd;
>
> + if (quick_prune)
> + flags |= QUICK_PRUNE;
> +
> if (show_only) {
> - write_shallow_commits_1(, 0, NULL, SEEN_ONLY | VERBOSE);
> + flags |= VERBOSE;
> + write_shallow_commits_1(, 0, NULL, flags);
> strbuf_release();
> return;
> }
> @@ -388,7 +400,7 @@ void prune_shallow(int show_only)
> git_path_shallow(the_repository),
> LOCK_DIE_ON_ERROR);
> check_shallow_file_for_update(the_repository);
> - if (write_shallow_commits_1(, 0, NULL, SEEN_ONLY)) {
> + if (write_shallow_commits_1(, 0, NULL, flags)) {
> if (write_in_full(fd, sb.buf, sb.len) < 0)
> die_errno("failed to write to %s",
> get_lock_file_path(_lock));
Re: [PATCH v3 1/3] repack: point out a bug handling stale shallow info
"Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > A `git fetch --prune` can turn previously-reachable objects unreachable, > even commits that are in the `shallow` list. A subsequent `git repack > -ad` will then unceremoniously drop those unreachable commits, and the > `shallow` list will become stale. This means that when we try to fetch > with a larger `--depth` the next time, we may end up with: > > fatal: error in object: unshallow Nicely analysed and described. One minor thing nagged me in the implementation but it is not a big issue. > +... > + d="$(git -C shallow-server rev-parse --verify D)" && > + git -C shallow-server checkout master && > + > +... > + ! grep $d shallow-client/.git/shallow && We know D (and $d) is not a tag, but perhaps the place that assigns to $d (way above) can do the rev-parse of D^0, not D, to make it more clear what is going on, especially given that... > + git -C shallow-server branch branch-orig D^0 && ... this does that D^0 thing here to makes us wonder if D needs unwrapping before using it as a commit (not commit-ish). If we did so, we could use $d here instead of D^0. > + git -C shallow-client fetch --prune --depth=2 \ > + origin "+refs/heads/*:refs/remotes/origin/*" > +' > + > . "$TEST_DIRECTORY"/lib-httpd.sh > start_httpd
Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed
Matthew DeVore writes:
> On Tue, 23 Oct 2018, Junio C Hamano wrote:
>
>> Not really. We were already doing a controlled failure via die(),
>> so these two tests would not have caught the problem in the code
>> before the fix in this patch.
>>
>
> BUG is apparently considered a "wrong" failure and not a controlled one
> by test_must_fail. I just double-checked that the tests fail without
> this patch.
Ah, I was testing a wrong codepath. Yes, it does call BUG("..."),
which is a prettier-looking abort(), but I somehow thought it was
doing die("BUG: ...").
In any case, thanks for the fix.
Re: [PATCH v2] compat: make sure git_mmap is not expected to write
Carlo Marcelo Arenas Belón writes:
> in f48000fc ("Yank writing-back support from gitfakemmap.", 2005-10-08)
> support for writting back changes was removed but the specific prot
> flag that would be used was not checked for
>
> Acked-by: Johannes Schindelin
> Signed-off-by: Carlo Marcelo Arenas Belón
> ---
> Changes in v2:
>
> * reset-author to match signature
> * cleanup commit message and add ACK
Thanks. Looking good.
> compat/mmap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/compat/mmap.c b/compat/mmap.c
> index 7f662fef7b..14d31010df 100644
> --- a/compat/mmap.c
> +++ b/compat/mmap.c
> @@ -4,7 +4,7 @@ void *git_mmap(void *start, size_t length, int prot, int
> flags, int fd, off_t of
> {
> size_t n = 0;
>
> - if (start != NULL || !(flags & MAP_PRIVATE))
> + if (start != NULL || flags != MAP_PRIVATE || prot != PROT_READ)
> die("Invalid usage of mmap when built with NO_MMAP");
>
> start = xmalloc(length);
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
Stefan Beller writes: >> Anyway, even though "make coccicheck" does not run in subsecond, >> I've updated my machinery to rebuild the integration branches so >> that I can optionally queue generated coccicheck patches, and what I >> pushed out tonight has one at the tip of 'pu' and also another at >> the tip of 'next'. The latter seems to be passing all archs and >> executing Windows run. > > That is pretty exciting! > > Looking at the commit in next, you also included the suggestion > from [1] to use a postincrement instead of a preincrement and I got > excited to see how we express such a thing in coccinelle, > but it turns out that it slipped in unrelated to the coccinelle patches. See below, which was sitting in my working tree. > How would we go from here? > It is not obvious to me how such changes would be integrated, > as regenerating them on top of pu will not help getting these changes > merged down, and applying the semantic patch on next (once > sb/more-repo-in-api lands in next) would created the merge conflicts for > all topics that are merged to next after that series. Conflicts with later topics is indeed worrysome. That is why I did it as an experiment. If it becomes too painful, I'd probably stop doing it while merging to anything other than 'pu', and then we can follow the more distributed approach along the lines of what Szeder suggested, to see how smoothly it goes. -- >8 -- Subject: [PATCH] cocci: simplify "if (++u > 1)" to "if (u++)" It is more common to use post-increment than pre-increment when the side effect is the primary thing we want in our code and in C in general (unlike C++). Initializing a variable to 0, incrementing it every time we do something, and checking if we have already done that thing to guard the code to do that thing, is easier to understand when written if (u++) ; /* we've done that! */ else do_it(); /* just once. */ but if you try to use pre-increment, you end up with a less natural looking if (++u > 1) Signed-off-by: Junio C Hamano --- contrib/coccinelle/preincr.cocci | 5 + 1 file changed, 5 insertions(+) create mode 100644 contrib/coccinelle/preincr.cocci diff --git a/contrib/coccinelle/preincr.cocci b/contrib/coccinelle/preincr.cocci new file mode 100644 index 00..7fe1e8d2d9 --- /dev/null +++ b/contrib/coccinelle/preincr.cocci @@ -0,0 +1,5 @@ +@ preincrement @ +identifier i; +@@ +- ++i > 1 ++ i++ -- 2.19.1-542-gc4df23f792
Re: [PATCH] range-diff: allow to diff files regardless submodule
Lucas De Marchi writes: >> Yes, I agree on both counts (i.e. it was totally unclear what >> problem is being solved and what the root cause of the problem is, >> and we would want a new test to protect this "fix" from getting >> broken in the future. > > have you seen I sent a v2 with proper test? No, otherwise I wouln't have said it needs tests, and no, because I haven't seen the v2, I do not know if it came with proper test or other issues pointed out and fixes suggested in the review round were addressed in v2. Sorry. When you ask such a question, please accompany it with "this is the message-id" to avoid the receiver of the question locating a wrong version of your patch from the archive. Thanks.
Re: [PATCH 1/3] gpg-interface.c: use flags to determine key/signer info presence
"brian m. carlson" writes: > On Mon, Oct 22, 2018 at 06:38:19PM +0200, Michał Górny wrote: >> Replace the logic used to determine whether key and signer information >> is present to use explicit flags in sigcheck_gpg_status[] array. This >> is more future-proof, since it makes it possible to add additional >> statuses without having to explicitly update the conditions. > > This series looks good to me. I was going to ask after patch 2 whether > you were printing the subkey or primary key fingerprint, and then you > answered my question in patch 3. Thanks for including both. Yeah, this looks good to me too. Thanks, both.
Re: [PATCH] test: avoid failures when USE_NED_ALLOCATOR
Carlo Marcelo Arenas Belón writes:
> contrib/nedmalloc doesn't support MALLOC_CHECK_ or MALLOC_PERTURB_
> so add it to the same exception that is being used with valgrind
>
> Signed-off-by: Carlo Marcelo Arenas Belón
> ---
> t/test-lib.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Unlike the case for valgrind, where we actively do not want to set
these two environment variables [*1*], I am assuming that nedmalloc
simply ignores these two environment variables. Is there a reason
why we want to special case nedmalloc like this patch does, but
leave these set for other implementations that do not pay attention
to them?
Of course, I am also assuming that there are implementations of
malloc(3), which are not nedmalloc, that can be used to build and
test Git and that do not react to these two environment variables.
The patch would make sense if all the other implementations of
malloc(3) paid attention to MALLOC_{CHECK,PERTURB}_ and nedmalloc
were the only odd-man out, but I do not think that is the case.
[Footnote]
*1* see the last two paragraphs of the log message of a731fa91 ("Add
MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the test suite for
detecting heap corruption", 2012-09-14)
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 44288cbb59..2ad9e6176c 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -143,7 +143,7 @@ fi
> # Add libc MALLOC and MALLOC_PERTURB test
> # only if we are not executing the test with valgrind
> if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
> - test -n "$TEST_NO_MALLOC_CHECK"
> + test -n "$TEST_NO_MALLOC_CHECK" || test -n "$USE_NED_ALLOCATOR"
> then
> setup_malloc_check () {
> : nothing
Re: [PATCH/RFC] thread-utils: better wrapper to avoid #ifdef NO_PTHREADS
Jeff King writes: > I also think we may want to make a fundamental shift in our view of > thread support. In the early days, it was "well, this is a thing that > modern systems can take advantage of for certain commands". But these > days I suspect it is more like "there are a handful of legacy systems > that do not even support threads". > > I don't think we should break the build on those legacy systems, but > it's probably OK to stop thinking of it as "non-threaded platforms are > the default and must pay zero cost" and more as "threaded platforms are > the default, and non-threaded ones are OK to pay a small cost as long as > they still work". Good suggestion.
Re: [PATCH v3 2/3] reset: add new reset.quiet config setting
Ben Peart writes: >>> diff --git a/Documentation/config.txt b/Documentation/config.txt >>> index f6f4c21a54..a2d1b8b116 100644 >>> --- a/Documentation/config.txt >>> +++ b/Documentation/config.txt >>> @@ -2728,6 +2728,9 @@ rerere.enabled:: >>> `$GIT_DIR`, e.g. if "rerere" was previously used in the >>> repository. >>> >>> +reset.quiet:: >>> + When set to true, 'git reset' will default to the '--quiet' option. >>> + >> >> With 'nd/config-split' topic moving pretty much all config keys out of >> config.txt, you probably want to do the same for this series: add this >> in a new file called Documentation/reset-config.txt then include the >> file here like the sendemail one below. >> > > Seems a bit overkill to pull a line of documentation into a separate > file and replace it with a line of 'import' logic. Perhaps if/when > there is more documentation to pull out that would make more sense. This change (ehh, rather, perhaps nd/config-split topic) came at an unfortunate moment. Until I actually did one integration cycle to rebuild 'pu' and merge this patch and the other topic, I had exactly the same reaction as yours above to Duy's comment. But seeing the tree at the tip of 'pu' today, I do think the end result with a single liner file that has configuration for the "reset" command that is included in config.txt would make sense, and I also think you would agree with it if you see the same tree. How we should get there is a different story. I think Duy's series needs at least another update to move the split pieces into its own subdirectory of Documentation/, and it is not all that urgent, while this three-patch series (with the advice.* bit added) for "reset" is pretty much ready to go 'next', so my gut feeling is that it is best to keep the description here, and to ask Duy to base the updated version of config-split topic on top.
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
Slavica writes: > +test_expect_failure 'stash with HOME as non-existing directory' ' > +test_commit 1 && > +test_config user.useconfigonly true && > +test_config stash.usebuiltin true && > +( > +HOME=$(pwd)/none && > +export HOME && What is the reason why this test needs to move HOME away from TRASH_DIRECTORY (set in t/test-lib.sh)? > +unset GIT_AUTHOR_NAME && > +unset GIT_AUTHOR_EMAIL && > +unset GIT_COMMITTER_NAME && > +unset GIT_COMMITTER_EMAIL && > +test_must_fail git config user.email && > +echo changed >1.t && > + git stash > +) > +' > + > test_done
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
Carlo Arenas writes: > On Tue, Oct 23, 2018 at 2:40 AM Junio C Hamano wrote: >> >> The tip of 'pu' has trouble with -Wunused on Apple around the >> delta-islands series. > > FWIW the "problem" is actually with -Wunused-function and is AFAIK not > related to the semantic changes or Apple (AKA macOS) Oh, don't get me wrong. By Apple, I meant "the versions of compiler used on the Apple build at TravisCI site". I could have sent the above two lines in a separate topic, as the issue does not have anything to do with "new semantic patches" discussion, but I thought it would be obvious to everybody who is reading the thread---it seems I thought wrong ;-)
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
Junio C Hamano writes: > I actually think this round does a far nicer job playing well with > other topics than any earlier series. The pain you are observing I > think come primarily from my not making the best use of these > patches. > > Steppng back a bit, I'd imagine in an ideal world where "make > coccicheck" can be done instantaneously _and_ the spatch machinery > is just as reliable as C compilers > > What I _could_ do (and what I did do only for one round of pushing > out 'pu') is to queue a coccinelle transformation at the tip of > integration branches. If "make coccicheck" ran in subsecond, I > could even automate it in the script that is used to rebuild 'pu' > every day, ... Anyway, even though "make coccicheck" does not run in subsecond, I've updated my machinery to rebuild the integration branches so that I can optionally queue generated coccicheck patches, and what I pushed out tonight has one at the tip of 'pu' and also another at the tip of 'next'. The latter seems to be passing all archs and executing Windows run. The tip of 'pu' has trouble with -Wunused on Apple around the delta-islands series. next: https://travis-ci.org/git/git/builds/444969406 pu: https://travis-ci.org/git/git/builds/444969342
Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed
Matthew DeVore writes: > t/t4202-log.sh | 4 > t/t8002-blame.sh | 4 > 7 files changed, 14 insertions(+), 1 deletion(-) > ... > diff --git a/t/t4202-log.sh b/t/t4202-log.sh > index 153a506151..819c24d10e 100755 > --- a/t/t4202-log.sh > +++ b/t/t4202-log.sh > @@ -1703,4 +1703,8 @@ test_expect_success 'log --source paints symmetric > ranges' ' > test_cmp expect actual > ' > > +test_expect_success '--exclude-promisor-objects does not BUG-crash' ' > + test_must_fail git log --exclude-promisor-objects source-a > +' > + > test_done > diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh > index 380e1c1054..eea048e52c 100755 > --- a/t/t8002-blame.sh > +++ b/t/t8002-blame.sh > @@ -118,4 +118,8 @@ test_expect_success '--no-abbrev works like --abbrev=40' ' > check_abbrev 40 --no-abbrev > ' > > +test_expect_success '--exclude-promisor-objects does not BUG-crash' ' > + test_must_fail git blame --exclude-promisor-objects one > +' > + > test_done OK. We used to be hitting BUG() which is an abort() in disguise, so must-fail would have caught it without the fix in this patch. Now we would see a more controlled failure. ... goes and makes sure that is the case ... Not really. We were already doing a controlled failure via die(), so these two tests would not have caught the problem in the code before the fix in this patch. But nevertheless this is a good change; I do not think it is worth grepping for "unrecognized option" to differentiate the two cases. Thanks.
Re: [RFC 0/2] explicitly support or not support --exclude-promisor-objects
Matthew DeVore writes: > This patch set fixes incorrect parsing of the --exclude-promisor-objects > option that I found while working on: > > https://public-inbox.org/git/cover.1539298957.git.matv...@google.com/ > Thanks; both patches make sense. As the problematic feature appeared in 2.17.x track, I'll see if I can easily make it ready to be merged down to maint-2.17 track later when somebody wants to. > Matthew DeVore (2): > Documentation/git-log.txt: do not show --exclude-promisor-objects > exclude-promisor-objects: declare when option is allowed > > Documentation/rev-list-options.txt | 2 +- > builtin/pack-objects.c | 1 + > builtin/prune.c| 1 + > builtin/rev-list.c | 1 + > revision.c | 3 ++- > revision.h | 1 + > t/t4202-log.sh | 4 > t/t8002-blame.sh | 4 > 8 files changed, 15 insertions(+), 2 deletions(-)
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
Stefan Beller writes: > Am I overestimating or misunderstanding rerere here? Yes. > Would it be realistic for next and master branch instead of pu? > > I'd be wary for the master branch, as we may not want to rely on > spatch without review. (It can produce funny white space issues, > but seems to produce working/correct code) Yes, that is why I think it is a bit too late to do the "fixup with spatch" after merging to 'next' and 'master'.
Re: [PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash
SZEDER Gábor writes: >> To prevent that from happening, let's append `^0` after the stash hash, >> to make sure that it is interpreted as an OID rather than as a number. > > Oh, this is clever. Yeah, we can do this as we know we'd be dealing with a commit-ish. If we made a mistake to use a tree object to represent a stash, this trick wouldn't have been possible ;-) > FWIW, all patches look good to me, barring the typo below. > ... >> +/* Ensure that the hash is not mistake for a number */ > > s/mistake/mistaken/ Will squash this in and add your reviewed-by before queuing. Thanks, both.
Re: [PATCH v3] archive: initialize archivers earlier
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. > @@ -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". Thanks.
Re: [PATCH v2] 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. I wonder if it would be more productive to introduce a mechanism that can be used to address that use case more directly. For example, would it help to teach "git send-email" that sendemail.smtpuser set to a particular value (say '!', or empty string if you prefer) is equivalent to the variable unset at all?
Re: [PATCH v3 3/3] reset: warn when refresh_index() takes more than 2 seconds
Ben Peart writes: > From: Ben Peart > > refresh_index() is done after a reset command as an optimization. Because > it can be an expensive call, warn the user if it takes more than 2 seconds > and tell them how to avoid it using the --quiet command line option or > reset.quiet config setting. I am moderately negative on this step. It will irritate users who know about and still choose not to use the "--quiet" option, because they want to gain performance in later real work and/or they want to know what paths are now dirty. A working tree that needs long time to refresh will take long time to instead do "cached stat info says it may be modified so let's run 'diff' for real---we may discover that there wasn't any change after all" when a "git diff" is run after a "reset --quiet" that does not refresh; i.e. there would be valid reasons to run "reset" without "--quiet". It feels a bit irresponsible to throw an ad without informing pros-and-cons and to pretend that we are advising on BCP. In general, we do *not* advertise new features randomly like this. Thanks. The previous two steps looks quite sensible.
Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter
Jeff King writes:
> If nobody uses it, should we drop the return value, too? Like:
Yup.
>
> diff --git a/read-cache.c b/read-cache.c
> index 78c9516eb7..4b44a2eae5 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2052,12 +2052,11 @@ static void *load_cache_entries_thread(void *_data)
> return NULL;
> }
>
> -static unsigned long load_cache_entries_threaded(struct index_state *istate,
> const char *mmap, size_t mmap_size,
> - int nr_threads, struct
> index_entry_offset_table *ieot)
> +static void load_cache_entries_threaded(struct index_state *istate, const
> char *mmap, size_t mmap_size,
> + int nr_threads, struct
> index_entry_offset_table *ieot)
> {
> int i, offset, ieot_blocks, ieot_start, err;
> struct load_cache_entries_thread_data *data;
> - unsigned long consumed = 0;
>
> /* a little sanity checking */
> if (istate->name_hash_initialized)
> @@ -2115,12 +2114,9 @@ static unsigned long
> load_cache_entries_threaded(struct index_state *istate, con
> if (err)
> die(_("unable to join load_cache_entries thread: %s"),
> strerror(err));
> mem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);
> - consumed += p->consumed;
> }
>
> free(data);
> -
> - return consumed;
> }
> #endif
>
>
> -Peff
Re: [PATCH] Poison gettext with the Ook language
Ævar Arnfjörð Bjarmason writes: > So I think the only reason to keep it compile-time is performance, but I > don't think that matters. It's not like we're printing gigabytes of _() > formatted output. Everything where formatting matters is plumbing which > doesn't use this API. These messages are always for human consumption. > > So shouldn't we just drop this notion of GETTEXT_POISON at compile-time > entirely, and make GIT_GETTEXT_POISON= a test flag that all > builds of git are aware of? I think so. Haven't thought things through, but that sounds like a good thing to aim for. Keep the ideas coming...
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
SZEDER Gábor writes:
> I don't really like how this or the previous RFC patch series deal
> with semantic patches (or how some past patch series dealt with them,
> for that matter), for various reasons:
> ...
> How about introducing the concept of "pending" semantic patches,
> stored in 'contrib/coccinelle/.pending.cocci' files, modifying
> 'make coccicheck' to skip them, and adding the new 'make
> coccicheck-pending' target to make it convenient to apply them, e.g.
> something like the simple patch at the end.
>
> So the process would go something like this:
>
> - A new semantic patch should be added as "pending", e.g. to the
> file 'the_repository.pending.cocci', together with the resulting
> transformations in the same commit.
>
> This way neither 'make coccicheck' nor the static analysis build
> job would complain in the topic branch or in the two integration
> branches. And if they do complain, then we would know right away
> that they complain because of a well-established semantic patch.
> Yet, anyone interested could run 'make coccicheck-pending' to see
> where are we heading.
>
> - The author of the "pending" semanting patch should then keep an
> eye on already cooking topics: whether any of them contain new
> code that should be transformed, and how they progress to
> 'master', and sending followup patch(es) with the remaining
> transformations when applicable.
>
> Futhermore, the author should also pay attention to any new topics
> that branch off after the "pending" semantic patch, and whether
> any of them introduce code to be transformed, warning their
> authors as necessary.
>
> - Finally, after all the dust settled, the dev should follow up with
> a patch to:
>
> - promote the "penging" patch to '.cocci', if its purpose
> is to avoid undesirable code patterns in the future, or
>
> - remove the semantic patch, if it was used in a one-off
> transformation.
> ...
> Thoughts?
I actually think this round does a far nicer job playing well with
other topics than any earlier series. The pain you are observing I
think come primarily from my not making the best use of these
patches.
Steppng back a bit, I'd imagine in an ideal world where "make
coccicheck" can be done instantaneously _and_ the spatch machinery
is just as reliable as C compilers. In such a world, I may rename
all the *.c files in my tree to *.c.in, make the very first step of
the "make all" to run coccicheck and transform *.c.in to *.c before
starting the compilation. There is no need to have changes to
*.c.in that spatch would fix. Such an idealized set-up has a few
nice property.
- Mechanical conflict resolutions between topics in flight and a
series that modifies or adds new .cocci rules would greatly be
reduced.
- Still, *.c files that are "compiled" from *.c.in file used by
each build will by definition cocci-clean.
- Bugs like the one that required 6afedba8 ("object_id.cocci: match
only expressions of type 'struct object_id'", 2018-10-15) are
still possible, but some of them may be caught by C compilers
that inspect the result of spatch compilation from *.c.in to *.c
Now we do not live in that ideal world and there is no separate
"turn *.c.in into *.c" step in our build procedure. In such a
"real" world, if we had a rule like "each individual commit must
pass 'make coccicheck' cleanly", anybody who does such a merge and
resolve huge conflics would essentially be wasting time on something
that the tool could do, and then the result of the merge would
further need to be amended for semantic conflicts (i.e. the other
branch may have introduced new instances of old pattern .cocci
patches in our branch would want to transform)). By not insisting
on cocci-cleanness at each commit level, we can gain (or at least
sumulate gaining) some benefit that we would reap in the ideal
world, and Stefan's latest series already does so for the former.
If we insisted that these patches must be accompanied with the
result of the cocci transformations, such a series will have zero
chance of landing in 'pu', unless we freeze the world.
What I _could_ do (and what I did do only for one round of pushing
out 'pu') is to queue a coccinelle transformation at the tip of
integration branches. If "make coccicheck" ran in subsecond, I
could even automate it in the script that is used to rebuild 'pu'
every day, so that after merging each and every topic, do the "make
coccicheck" and apply resulting .cocci.patch files and squash that
into the merge commit.
But with the current "make coccicheck" performance, that is not
realistic.
I am wondering if it is feasible to do it at the tip of 'pu' (which
is rebuilt two to three times a day), 'next' (which is updated once
or twice a week) and 'master'.
I find that your "pending" idea may be nicer, as it distributes the
load. Whoever wants to change the world order
Re: [PATCH 2/3] git-compat-util: define MIN through compat
Carlo Marcelo Arenas Belón writes:
> this macro is commonly defined in system headers (usually )
> but if it is not define it here so it can be used elsewhere
>
> Signed-off-by: Carlo Marcelo Arenas Belón
> ---
I am between "meh" and "moderately negative" on this change.
- Definition of MIN without matching MAX makes me worried about
longer-term maintainability. If we were to add MIN() we should
also add MAX() to match.
However, "git grep -e '#define MAX(' -e '#define MIN('" tells me
that we do not use these anywhere and Brian's use of a single
MIN() is the only thing that makes this patch interesting. That
is the primary reason why I am very close to "meh" on this.
- We need to make sure that this is enough in "git-compat-util.h".
Ideally, this should be after all "#include" of the system
supplied header files, and before any use of MIN() macro
ourselves. I am reasonably sure that the place this patch chose
to insert the definition satisfies that criterion within the
context of the today's code, but I am unsure if it is even worth
having to worry about it in the first place, given how rarely if
ever the macro is used. I think Brian's reroll of the sha256
series even gets rid of the use of the macro, as it had only a
single place that used the macro anyway.
So...
> git-compat-util.h | 5 +
> sha256/block/sha256.c | 3 ---
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 5f2e90932f..7a0b48452b 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -218,6 +218,11 @@
> #else
> #include
> #endif
> +
> +#ifndef MIN
> +#define MIN(x, y) ((x) < (y) ? (x) : (y))
> +#endif
> +
> #ifdef NO_INTPTR_T
> /*
> * On I16LP32, ILP32 and LP64 "long" is the safe bet, however
> diff --git a/sha256/block/sha256.c b/sha256/block/sha256.c
> index 0d4939cc2c..5fba943c79 100644
> --- a/sha256/block/sha256.c
> +++ b/sha256/block/sha256.c
> @@ -130,9 +130,6 @@ static void blk_SHA256_Transform(blk_SHA256_CTX *ctx,
> const unsigned char *buf)
> }
> }
>
> -#ifndef MIN
> -#define MIN(x, y) ((x) < (y) ? (x) : (y))
> -#endif
> void blk_SHA256_Update(blk_SHA256_CTX *ctx, const void *data, size_t len)
> {
> const unsigned char *in = data;
Re: [PATCH 1/3] sha256: avoid redefinition for MIN
Carlo Marcelo Arenas Belón writes:
> it is already defined whenever "sys/param.h" is available
>
> Signed-off-by: Carlo Marcelo Arenas Belón
> ---
> sha256/block/sha256.c | 2 ++
> 1 file changed, 2 insertions(+)
It is a no-brainer to say that this is obviously good. I'd rather
see this become a part of sha256 topic (perhaps squash it in to
eliminate the need to have it as a separate patch), though.
Thanks.
>
> diff --git a/sha256/block/sha256.c b/sha256/block/sha256.c
> index 18350c161a..0d4939cc2c 100644
> --- a/sha256/block/sha256.c
> +++ b/sha256/block/sha256.c
> @@ -130,7 +130,9 @@ static void blk_SHA256_Transform(blk_SHA256_CTX *ctx,
> const unsigned char *buf)
> }
> }
>
> +#ifndef MIN
> #define MIN(x, y) ((x) < (y) ? (x) : (y))
> +#endif
> void blk_SHA256_Update(blk_SHA256_CTX *ctx, const void *data, size_t len)
> {
> const unsigned char *in = data;
Re: [PATCH 3/3] xdiff: use compat's MIN instead
Carlo Marcelo Arenas Belón writes: > Signed-off-by: Carlo Marcelo Arenas Belón > --- > xdiff/xdiffi.c | 2 +- > xdiff/xemit.c | 6 +++--- > xdiff/xhistogram.c | 6 +++--- > xdiff/xmacros.h| 4 +--- > xdiff/xprepare.c | 2 +- > 5 files changed, 9 insertions(+), 11 deletions(-) As this is a borrowed code, I'd rather not do this, especially do so only for MIN so that people who update this need to be aware that they can say MIN() but still have to say XDL_MAX(). > - > -#define XDL_MIN(a, b) ((a) < (b) ? (a): (b)) > #define XDL_MAX(a, b) ((a) > (b) ? (a): (b))
Re: [PATCH] fetch-pack: be more precise in parsing v2 response
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 &&
Re: Mirror of git.git on gitlab.com
Ævar Arnfjörð Bjarmason writes: > I've set this up at https://gitlab.com/git-vcs > > The /git namespace was taken (and I asked GitLab support if it was > stale, they said no). Also tried /git-scm and /gitscm, ditto. So I > settled on /git-vcs (version control system). Squatters X-<. Thanks. > As an aside, I noticed that > https://github.com/git/sha1collisiondetection/ has never worked in > combination with git.git, i.e. it's cloned at a version that pre-dates > the initial introduction of the sha1collisiondetection submodule. Our > other mirrors don't seem to have it at all relative to > ../sha1collisiondetection.git from their git.git. I do not recall who cloned it or forked it there or what our longer term plans for that repository would be. I think we are using this thing: https://github.com/cr-marcstevens/sha1collisiondetection.git and there is probably no reason to have our own copy. Perhaps we should just get rid of it.
Re: [PATCH v2 2/3] reset: add new reset.quiet config setting
Ben Peart writes: > On 10/19/2018 1:11 PM, Jeff King wrote: >> On Fri, Oct 19, 2018 at 01:10:34PM -0400, Eric Sunshine wrote: >> >>> On Fri, Oct 19, 2018 at 12:46 PM Jeff King wrote: On Fri, Oct 19, 2018 at 12:36:44PM -0400, Eric Sunshine wrote: > How does the user reverse this for a particular git-reset invocation? > There is no --no-quiet or --verbose option. > > Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in > builtin/reset.c and document that --verbose overrides --quiet and > reset.quiet (or something like that). I think OPT__QUIET() provides --no-quiet, since it's really an OPT_COUNTUP() under the hood. Saying "--no-quiet" should reset it back to 0. >>> >>> Okay. In any case, --no-quiet probably ought to be mentioned alongside >>> the "reset.quiet" option (and perhaps in git-reset.txt to as a way to >>> reverse "reset.quiet"). >> >> Yes, I'd agree with that. >> >> -Peff >> > > Makes sense. I'll update the docs to say: > > -q:: > --quiet:: > --no-quiet:: > Be quiet, only report errors. > + > With --no-quiet report errors and unstaged changes after reset. Sounds good. Thanks all.
Re: [PATCH v4 2/2] worktree: add per-worktree config files
Nguyễn Thái Ngọc Duy writes: > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 552827935a..244560a35e 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -2,8 +2,9 @@ CONFIGURATION FILE > -- > > The Git configuration file contains a number of variables that affect > -the Git commands' behavior. The `.git/config` file in each repository > -is used to store the configuration for that repository, and > +the Git commands' behavior. The files `.git/config` and optionally > +`config.worktree` (see `extensions.worktreeConfig` below) in each > +repository are used to store the configuration for that repository, and > `$HOME/.gitconfig` is used to store a per-user configuration as > fallback values for the `.git/config` file. The file `/etc/gitconfig` > can be used to store a system-wide default configuration. > @@ -371,6 +372,13 @@ advice.*:: > editor input from the user. > -- > > +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)? - 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?
Re: [PATCH v3 2/8] Add a place for (not) sharing stuff between worktrees
Nguyễn Thái Ngọc Duy writes: > Subject: Re: [PATCH v3 2/8] Add a place for (not) sharing stuff between > worktrees "a place"? Missing "in $GIR_DIR" in the descrition made me read the above three times before getting what it wanted to say. My attempt to improve it, which admittedly is not great, came up with: worktree: convention to make per-worktree things identifiable in $GIT_DIR > When multiple worktrees are used, we need rules to determine if > something belongs to one worktree or all of them. Instead of keeping > adding rules when new stuff comes (*), have a generic rule: > > - Inside $GIT_DIR, which is per-worktree by default, add > $GIT_DIR/common which is always shared. New features that want to > share stuff should put stuff under this directory. > > - Inside refs/, which is shared by default except refs/bisect, add > refs/worktree/ which is per-worktree. We may eventually move > refs/bisect to this new location and remove the exception in refs > code. > > (*) And it may also include stuff from external commands which will > have no way to modify common/per-worktree rules. OK. Establishing such a convention is a good role for the core-git should play to help third-party tools. Should this play well with the per-worktree configuration as well? Is it better to carve out a configuration variable namespace so that certain keys are never read from common ones (or per-worktree ones), so that people can tell which ones are what? I know your current design says "this is just another new layer, and the users can hang themselves with this new rope". I am wondering if there is a need to do something a bit more structured.
Re: [PATCH] completion: fix __gitcomp_builtin no longer consider extra options
Nguyễn Thái Ngọc Duy writes:
> __gitcomp_builtin() has the main completion list provided by
>
> git xxx --git-completion-helper
>
> but the caller can also add extra options that is not provided by
> --git-completion-helper. The only call site that does this is "git
> difftool" completion.
>
> This support is broken by b221b5ab9b (completion: collapse extra
> --no-.. options - 2018-06-06), which adds a special value "--" to mark
> that the rest of the options can be hidden by default. The commit
> forgets the fact that extra options are appended after
> "$(git xxx --git-completion-helper)", i.e. after this "--", and will
> be incorrectly hidden as well.
>
> Prepend the extra options before "$(git xxx --git-completion-helper)"
> to avoid this.
Thanks for a clear analysis. How did you find it? Got annoyed that
completion of difftool got broken, or discovered while trying to
apply the same trick as difftool completion uses to another one and
seeing that the technique does not work?
Will queue. Thanks.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> contrib/completion/git-completion.bash | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index db7fd87b6b..c8fdcf8644 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -400,7 +400,7 @@ __gitcomp_builtin ()
> if [ -z "$options" ]; then
> # leading and trailing spaces are significant to make
> # option removal work correctly.
> - options=" $(__git ${cmd/_/ } --git-completion-helper) $incl "
> + options=" $incl $(__git ${cmd/_/ } --git-completion-helper) "
> for i in $excl; do
> options="${options/ $i / }"
> done
Re: [PATCH] read-cache: use of memory after it is freed
Duy Nguyen writes:
>> freshen_shared_index(base_path, 0);
>> merge_base_index(istate);
>> post_read_index_from(istate);
>> - free(base_path);
>> trace_performance_leave("read cache %s", base_path);
>> + free(base_path);
>
> Oops. Ack.
Thanks, both.
>> introduced with c46c406ae1e (trace.h: support nested performance tracing)
>> on Aug 18, 2018 but not affecting maint
Yup. The series nd/unpack-trees-with-cache-tree however was
designed to be merge-able down to 'maint', so it is a good idea to
fix this at the tip of the topic. So I'll queue it on top of
5f4436a7 ("Document update for nd/unpack-trees-with-cache-tree",
2018-08-25).
Re: [PATCH] builtin/receive-pack: dead initializer for retval in check_nonce
Torsten Bögershausen writes: > Initializing a variable to "BAD" in the beginning can be a good thing > for two reasons: > - There is a complex if-elseif chain, which should set retval > in any case, this is at least what I expect taking a very quick look at the > code: > ... > # The second reason is that some compilers don't understand this complex > # stuff either, and through out a warning, like > # "retval may be uninitialized" or something in that style. > # This is very compiler dependent. And to help humans that unless some if/else chain explicitly says it is OK, the caller receives BAD by default. In other words, it is being defensive. At least that was the reasoning behind the original code that did not support SLOP. > So yes, the current code may seem to be over-eager and ask for > optimization, but we don't gain more that a couple of nano-seconds > or so. The good thing is that we have the code a little bit more > robust, when changes are done in the future. True.
Re: [PATCH 1/1] archive: init archivers before determining format
Jeff King writes:
> On Fri, Oct 19, 2018 at 04:19:28PM -0700, stead...@google.com wrote:
>
>> diff --git a/builtin/archive.c b/builtin/archive.c
>> index e74f675390..dd3283a247 100644
>> --- a/builtin/archive.c
>> +++ b/builtin/archive.c
>> @@ -45,7 +45,10 @@ static int run_remote_archiver(int argc, const char
>> **argv,
>> * it.
>> */
>> if (name_hint) {
>> -const char *format = archive_format_from_filename(name_hint);
>> +const char *format;
>> +init_tar_archiver();
>> +init_zip_archiver();
>> +format = archive_format_from_filename(name_hint);
>> if (format)
>> packet_write_fmt(fd[1], "argument --format=%s\n",
>> format);
>
> Hrm. This code was added back in 56baa61d01 (archive: move file
> extension format-guessing lower, 2011-06-21), and your example
> invocation worked back then!
>
> Unfortunately it was broken by the very next patch in the series,
> 08716b3c11 (archive: refactor file extension format-guessing,
> 2011-06-21). I guess that's what I get for not adding regression tests.
>
> It's probably worth mentioning those points in the commit message.
>
> Does this work with configured archiver extensions, too? I think so,
> because we load them via init_tar_archiver().
>
> Can we avoid repeating the list of archivers here? This needs to stay in
> sync with the list in write_archive(). I know there are only two, but
> can we factor out an init_archivers() call or something?
>
> We also should probably just call it unconditionally when we start the
> archiver command (I don't think there are any other bugs like this
> lurking, but it doesn't cost very much to initialize these; it makes
> sense to just do it early).
>
> Other than those minor points (and the lack of test), your fix looks
> good to me.
Thanks for a patch and an excellent review. Looking forward to the
finalized version.
Thanks, both.
Re: git ls-files --with-tree documentation
Joey Hess writes:
> How about changing the documentation to something like this to make
> more explicit what it does.
>
>--with-tree=
>Treat all files in the as if they were present in the
> index.
>When using --error-unmatch to expand the user supplied (i.e.
>path pattern) arguments to paths, this has the effect that paths
> which were
>removed in the index since the named are still present.
>Using this option with -s or -u options does not make any sense.
If has a file F and the index has a file F/1, I do not
think the command can pretend that F is present in the index (which
requires it to also pretend that F/1 does not exist), so the above
description is not quite right---the description needs to be
tightened a bit, I am afraid [*1*].
But more importantly, given the fact that we needed piecemeal
fix-ups like 4b4e26d2 ("Teach ls-files --with-tree= to work
with options other than -c", 2008-11-16), and the fact that your
description above still mentions "incompatible with -s", I strongly
suspect that the implementation as-is would still *not* perform the
way you describe above. In some modes, it won't pretend as if all
of are present in the index.
And I do not think I care that much to respond to a bug report that
claims the above paragraph describes the way the command ought to
work, either, but apparently you do care much more than I do, so
perhaps you can respond to such bug reports whey they come and I do
not have to worry about them too much ;-)
[Footnote]
*1* It actually pretends that entries in are at stage #1,
all the originally unmerged entries are at stage #3, and shows
entries at stage #0 (i.e. merged entries in the original index)
and stage #1 (i.e. from ), but only those that do not
have corresponding stage #0 entries. That is why "-s" won't
make sense (i.e. from an entry being at stage #3, you cannot
tell if it were originally at stage #1, #2 or #3), and "-u"
won't make sense (i.e. ditto---and there is no good explanation
as to why entries appear at stage #1, other than the
real reason: this is only to be able to enumerate all paths that
are in and the index, so that error-unmatch can say
"Ah, that path is in the HEAD so it is not a typo" even for a
path that has been removed from the index when running "git
commit ").
Re: Bug with "git mv" and submodules, and with "git submodule add something --force"
Jonathan Nieder writes: > Stefan Beller wrote: > >> Maybe for now we can do with just an update of the documentation/bugs >> section and say we cannot move files in and out of submodules? > > I think we have some existing logic to prevent "git add"-ing a file > within a submodule to the superproject, for example. There is die_path_inside_submodule() that sanity-checks the pathspec and rejects. But I think that is done primarily to give an error message and not strictly necesary for correctness. The real safety of "git add" is its call to dir.c::fill_directory(); it collects untracked paths that match the pathspec so that they can be added as new paths, but because it won't cross the module boundary, you won't get such a path in the index to begin with. > So "git mv" should learn the same trick. And perhaps the trick needs > to be moved down a layer (e.g. into the index API). Hints? You would want to be able to remove a submodule and replace it with a directory, but you can probably do it in two steps, i.e. git reset --hard git rm --cached sha1collisiondetection echo Now a regular dir >sha1collisiondetection/READ.ME find sha1collisiondetection ! -type d -print0 | git update-index --add --stdin -z So from that point of view, forbidding (starting from the same state of our project) this sequence: git reset --hard echo Now a regular dir >sha1collisiondetection/READ.ME find sha1collisiondetection ! -type d -print0 | git update-index --add --remove --stdin -z that would nuke the submodule and replace it with a directory within which there are files would be OK. Making the latter's default rejection overridable with ADD_CACHE_OK_TO_REPLACE would also be fine.
Re: [PATCH 1/1] commit-reach: fix first-parent heuristic
Derrick Stolee writes: > On 10/19/2018 1:24 AM, Junio C Hamano wrote: >> "Derrick Stolee via GitGitGadget" writes: >> >>> We can also re-run the performance tests from commit 4fbcca4e >>> "commit-reach: make can_all_from_reach... linear". >>> >>> Performance was measured on the Linux repository using >>> 'test-tool reach can_all_from_reach'. The input included rows seeded by >>> tag values. The "small" case included X-rows as v4.[0-9]* and Y-rows as >>> v3.[0-9]*. This mimics a (very large) fetch that says "I have all major >>> v3 releases and want all major v4 releases." The "large" case included >>> X-rows as "v4.*" and Y-rows as "v3.*". This adds all release-candidate >>> tags to the set, which does not greatly increase the number of objects >>> that are considered, but does increase the number of 'from' commits, >>> demonstrating the quadratic nature of the previous code. >> These micro-benchmarks are interesting, but we should also remember >> to describe the impact of the bug being fixed in the larger picture. >> What end-user visible operations are affected? Computing merge-base? >> Finding if a push fast-forwards? Something else? > > Sorry about that. Here is a description that could be inserted into > the commit message: > > The can_all_from_reach() method synthesizes the logic for one > iteration of can_all_from_reach_with_flags() which is used by the > server during fetch negotiation to determine if more haves/wants are > needed. The logic is also used by is_descendant_of() which is used to > check if a force-push is required and in 'git branch --contains'. I am afraid that it is still not end-user serving enough. The level of "larger picture" I had in mind was those that would appear as an entry in the release notes, e.g. (this is for illustration purposes only; I do not claim its actual contents is correct). We started using commit generation numbers in various reachability computations, but due to a bug, negitiation between the "git fetch" and the server started to require 30% more roundtrips than necessary, and it has become less efficient to see if a commit is a descendant of another commit in certain cases, which has been corrected in this release. Thanks.
Re: [RFC PATCH 2/3] Documentation/git-rev-list: s///
Matthew DeVore writes: >> It is more like "this is a set operation across commits. We also >> show objects that are reachable from the commits in the resulting >> set and are not reachable from the commits in the set that were >> excluded when --objects option is given". >> > That would be correct though it wouldn't tell that you can use > "--objects ^foo-tree bar-tree." Yeah, but quite honestly, I consider that it is working by accident, not by design, in the current code (iow, you are looking at a behaviour of whatever the code happens to do). "rev-list" is pretty much set operation across commits, and anything that deals with a non commit-ish given from the command line is an afterthought at best, and happenstance in reality. I do not mean to say that the code must stay that way, though.
Re: [PATCH v8 7/7] read-cache: load cache entries on worker threads
Jeff King writes:
> On Wed, Oct 10, 2018 at 11:59:38AM -0400, Ben Peart wrote:
>
>> +static unsigned long load_cache_entries_threaded(struct index_state
>> *istate, const char *mmap, size_t mmap_size,
>> +unsigned long src_offset, int nr_threads, struct
>> index_entry_offset_table *ieot)
>
> The src_offset parameter isn't used in this function.
>
> In early versions of the series, it was used to feed the p->start_offset
> field of each load_cache_entries_thread_data. But after the switch to
> ieot, we don't, and instead feed p->ieot_start. But we always begin that
> at 0.
>
> Is that right (and we can drop the parameter), or should this logic:
>
>> +offset = ieot_start = 0;
>> +ieot_blocks = DIV_ROUND_UP(ieot->nr, nr_threads);
>> +for (i = 0; i < nr_threads; i++) {
>> [...]
>
> be starting at src_offset instead of 0?
I think "offset" has nothing to do with the offset into the mmapped
region of memory. It is an integer index into a (virtual) array
that is a concatenation of ieot->entries[].entries[], and it is
correct to count from zero. The value taken from that array using
the index is used to compute the offset into the mmapped region.
Unlike load_all_cache_entries() called from the other side of the
same if() statement in the same caller, this does not depend on the
fact that the first index entry in the mmapped region appears
immediately after the index-file header. It goes from the offsets
into the file that are recorded in the entry offset table that is an
index extension, so the sizeof(*hdr) that initializes src_offset is
not used by the codepath.
The number of bytes consumed, i.e. its return value from the
function, is not really used, either, as the caller does not use
src_offset for anything other than updating it with "+=" and passing
it to this function (which does not use it) when it calls this
function (i.e. when ieot extension exists--and by definition when
that extension exists extension_offset is not 0, so we do not make
the final load_index_extensions() call in the caller that uses
src_offset).
Re: [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable
Derrick Stolee writes: >> base-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf > I should explicitly mention that this base commit is different as > otherwise I will conflict with ds/multi-pack-verify with the new > prototype in midx.h. There indeed is a tiny textual conflict, and in this case it may not matter that much, but please make it a habit to refrain from doing such a rebase in general. It makes it impossible to compare the new round in the same context that the old round was inspected and has been tested, unless such a textual conflict avoidance is undone. A good rule of thumb is to build on the same base, attempt a trial merge to 'master' (and 'next' and 'pu' if you are inclined to), and see how bad a conflict you get. And if the conflict is something you can trivially resolve and the resolution would bring the code to the same state as you would get if you rebased, then you are better off not rebasing and let the maintainer deal with the merge. You cannot control what other contributor would do to the code while you are working on it, so having to resolve these tiny textual conflicts is not "an unnecessary added burden" to me (having to backport to see the new round in the same context as the old round is, though). Of course, if you truly depend on some recent addition that happend since your old base, please do not hesitate to rebase. Thanks.
Re: [PATCH] alias: detect loops in mixed execution mode
Jeff King writes: > I agree it's probably quite rare, if it exists at all. But I also wonder > how important looping alias protection is. It's also rare, and the > outcome is usually "gee, I wonder why this is taking so long? ^C". > > At least that's my instinct. I don't remember having run into this at > all myself (though certainly I have written my fair share of infinite > loops in other systems, like bash aliases, and that is what happened). Yup, that instict is shared with me, and I tend to prefer something based on a simple counter for that reason. > Would we print a long error message? I'd assume that we'd just recurse > for longer and print one error message that says: > > fatal: woah, you're 1000-levels deep in Git commands! > > That doesn't help the user find the recursion, but re-running with > GIT_TRACE=1 would make it pretty clear, I'd think. Thanks.
Re: [PATCH v4 4/7] revision.c: begin refactoring --topo-order logic
Jakub Narebski writes: > So if revs->limited is set (but not because revs->topo_order is set), > which means A..B queries, we will be still using the old algorithm. > All right, though I wonder if it could be improved in the future > (perhaps with the help of other graph labelling / indices than > generation numbers, maybe a positive-cut index). > > Do you have an idea why there is no improvement with the new code in > this case? I didn't get the impression that it would not be possible to improve the "--topo A..B" case by using generation numbers from this series. Isn't it just because the necessary code has not been written yet? In addition to what is needed for "--topo P1 P2 P3..." (all positive), limited walk needs to notice the bottom boundary and stop traversal. Having generation numbers would make it slightly easier than without, as you know that a positive commit you have will not be marked UNINTERESTING due to a negative commit whose ancestors have not been explored, as long as that negative commit has a higher generation number. But you still need to adjust the traversal logic to properly terminate upon hitting UNINTERESTING one, and also propagate the bit down the history, which is not needed at all if you only want to support the "positive only" case.
Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits
Michał Górny writes: >> Very minor point but by not using pre-increment, i.e. >> >> if (seen_exclusive_status++) >> goto found_duplicate_status; >> >> you can use the expression as a "have we already seen?" boolean, >> whic may probably be more idiomatic. >> >> The patch is good in the way written as-is, and this is so minor >> that it is not worth rerolling to only update this part. >> > > Sure, thanks. For the record, I've been taught to use pre-increment > whenever possible to avoid copying the variable but I suppose it doesn't > really matter here. Just a habit. Yes, it's a habit many C++ trained people spread; it just looks weird to see a pre-increment of a "have we done this once?" variable and end up comparing to see if it is strictly greater than 1 (i.e. have we reached 2 or more?).
Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits
Michał Górny writes:
> GnuPG supports creating signatures consisting of multiple signature
> packets. If such a signature is verified, it outputs all the status
> messages for each signature separately. However, git currently does not
> account for such scenario and gets terribly confused over getting
> multiple *SIG statuses.
>
> For example, if a malicious party alters a signed commit and appends
> a new untrusted signature, git is going to ignore the original bad
> signature and report untrusted commit instead. However, %GK and %GS
> format strings may still expand to the data corresponding
> to the original signature, potentially tricking the scripts into
> trusting the malicious commit.
>
> Given that the use of multiple signatures is quite rare, git does not
> support creating them without jumping through a few hoops, and finally
> supporting them properly would require extensive API improvement, it
> seems reasonable to just reject them at the moment.
>
> Signed-off-by: Michał Górny
> ---
> gpg-interface.c | 90 +++-
> t/t7510-signed-commit.sh | 26
> 2 files changed, 87 insertions(+), 29 deletions(-)
>
> Changes in v4:
> * switched to using skip_prefix(),
> * renamed the variable to seen_exclusive_status,
> * made the loop terminate early on first duplicate status seen.
Thanks for sticking to the topic and polishing it further. Looks
very good.
Will replace.
> + int seen_exclusive_status = 0;
> +
> + /* Iterate over all lines */
> + for (line = buf; *line; line = strchrnul(line+1, '\n')) {
> + while (*line == '\n')
> + line++;
> + /* Skip lines that don't start with GNUPG status */
> + if (!skip_prefix(line, "[GNUPG:] ", ))
> + continue;
> +
> + /* Iterate over all search strings */
> + for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
> + if (skip_prefix(line, sigcheck_gpg_status[i].check,
> )) {
> + if (sigcheck_gpg_status[i].flags &
> GPG_STATUS_EXCLUSIVE) {
> + if (++seen_exclusive_status > 1)
> + goto found_duplicate_status;
Very minor point but by not using pre-increment, i.e.
if (seen_exclusive_status++)
goto found_duplicate_status;
you can use the expression as a "have we already seen?" boolean,
whic may probably be more idiomatic.
The patch is good in the way written as-is, and this is so minor
that it is not worth rerolling to only update this part.
Thanks.
Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)
Alban Gruin writes:
> The error comes from the call to `git stash apply $stash_id' in
> builtin/rebase.c:261. When $stash_id only contains decimals and no
> letters, git-stash tries to apply stash@{$stash_id}[0][1]. Thas was not
> a real problem with the shell script, because it did not abbreviate the
> object id of the stashed commit, so it was very unlikely that the oid
> would contain only digits. builtin/rebase.c shortens the oid[2], making
> this problem more likely to occur.
OK, so make "rebase in C" a faithful conversion of the original, the
code that lead to builtin/rebase.c:261 must be fixed not to pass a
shortened oid. I suspect that internally it has a full object name,
so not shortening would be the right thing anyway, so regaredless of
this bug, it probably makes sense to do the fix.
But as you said, even the scripted version that passed the full
object name had a (10/16^40) chance of using a 40-hex that only has
[0-9] and caused the same breakage, so such a change to "rebase in
C" is sweeping the age old bug under the same rug, in the context of
discussing this particular bug.
I agree with you that it is a better fix to the problem to allow the
caller to say "I am giving an oid---it may (or may not---I do not
even bother to check) consist of only digits, but do not treat it as
an index to the stash reflog."
Re: [PATCH 00/59] Split config.txt
Ævar Arnfjörð Bjarmason writes:
> I had a slight bias against this when you started this, since I'm one of
> these odd people who don't mind ~20k line files if the line count isn't
> contributing to inherent complexity, e.g. in the case of config.txt you
> could just use the search function all in one file.
After typing "less Documentation/config.txt" and realizing that I
have to open another file (which one?) to find how we described the
push.default config, I am already experiencing a lot stronger bias
against this.
But I know it will pass. Once this ~60 patch series completes, my
irritation would peak, because at that point I would not be able to
even do "git grep push.config Documentation/config*", but I would no
longer be reaching for "less Documentation/config.txt" anymore at
that point. Once Documentation/$group-config.txt (which I think is
a mistake) are renamed to Documentation/$something/$group.txt, then
I know I can do "less Doc/$some/$gro" to get my ease
of use back. There will still be an annoyance caused by having to
open another file when reading description of branch..merge in
branch-config.txt and seeing a reference to push.default, though.
And the end result makes it impossible to place a description of a
new variable in a wrong section. It still is possible to mistakenly
insert a variable in a wrong place in the right section that
requires a fix like 8578037b ("config.txt: reorder blame stuff to
keep config keys sorted", 2018-08-04), but we do not fix all the
problems under the sky in one series ;-).
So after saying all of the above, I am moderately supportive of this
series.
Re: [PATCH v2 02/13] ci/lib.sh: encapsulate Travis-specific things
SZEDER Gábor writes: >> >> +if test true = "$TRAVIS" >> >> +then >> >> +... >> >> + export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save" >> >> + export GIT_TEST_OPTS="--verbose-log -x --immediate" >> >> +fi >> > ... > > $GIT_PROVE_OPTS and $GIT_TEST_OPTS, however, are only used in > 't/Makefile' but not in the build scripts, thus their values don't > show up in the build logs. > > I run some Travis CI builds with custom $GIT_TEST_OPTS, which, of > course, conflicted with this patch, and I messed up the conflict > resolution. I think I would have noticed sooner what went wrong, if > the value of $GIT_TEST_OPTS were visible in the build logs. > > I've found the output with 'set -x' sufficient so far, and don't think > that an explicit logging facility is worth it. That's an interesting perspective. I would have thoguht that the values of these two variables we can see above, that are constants without any substitution or customization, are the least interesting things to see in the log.
Re: [PATCH 07/15] sequencer: make sequencer_make_script() write its script to a strbuf
SZEDER Gábor writes:
>> if (entry)
>> -fprintf(out, "\n%c Branch %s\n", comment_line_char,
>> entry->string);
>> +strbuf_addf(out, "\n%c Branch %s\n", comment_line_char,
>> entry->string);
>> else
>> -fprintf(out, "\n");
>> +strbuf_addf(out, "\n");
>
> Please use plain strbuf_add() here.
FWIW, contrib/coccinelle/strbuf.cocci.patch gave us this:
diff -u -p a/sequencer.c b/sequencer.c
--- a/sequencer.c
+++ b/sequencer.c
@@ -4311,7 +4311,7 @@ static int make_script_with_merges(struc
if (entry)
strbuf_addf(out, "\n%c Branch %s\n", comment_line_char,
entry->string);
else
- strbuf_addf(out, "\n");
+ strbuf_addstr(out, "\n");
while (oidset_contains(, >object.oid) &&
!oidset_contains(, >object.oid)) {
Re: [PATCH 00/19] Bring more repository handles into our code base
Stefan Beller writes: > This rerolls sb/more-repo-in-api. > It applies on nd/the-index merged with ds/reachable and is available via > git fetch https://github.com/stefanbeller/git object-store-final-3 Thanks. Luckily we have both of these prerequisites in 'master' now, o hopefully this can be applied to 'master' directly and would play well when merged to 'next' and to 'pu'. Will queue and play with it a bit before sending comments.
What's cooking in git.git (Oct 2018, #04; Fri, 19)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. Two large set of topics on "rebase in C" and "rebase -i in C" are now in 'next'. A handful of improvements around "git help", "git grep", and "git status" are in 'master', as well as other internal changes. Note that "cf. " are not meant as "clear all of these and you are home free". They are primarily to prevent me from merging topics that are not ready to 'next' by mistake and remind me where to go back to read the last state of the discussion in the archive. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * bp/read-cache-parallel (2018-10-11) 7 commits (merged to 'next' on 2018-10-12 at ed6edde799) + read-cache: load cache entries on worker threads + ieot: add Index Entry Offset Table (IEOT) extension + read-cache: load cache extensions on a worker thread + config: add new index.threads config setting + eoie: add End of Index Entry (EOIE) extension + read-cache: clean up casting and byte decoding + read-cache.c: optimize reading index format v4 A new extension to the index file has been introduced, which allows the file to be read in parallel. * bp/rename-test-env-var (2018-09-28) 6 commits (merged to 'next' on 2018-10-12 at 201e451d20) + t: do not get self-test disrupted by environment warnings + preload-index: update GIT_FORCE_PRELOAD_TEST support + read-cache: update TEST_GIT_INDEX_VERSION support + fsmonitor: update GIT_TEST_FSMONITOR support + preload-index: use git_env_bool() not getenv() for customization + t/README: correct spelling of "uncommon" Some environment variables that control the runtime options of Git used during tests are getting renamed for consistency. * ds/commit-graph-leakfix (2018-10-07) 3 commits (merged to 'next' on 2018-10-12 at 8cc7f2f1e9) + commit-graph: reduce initial oid allocation + builtin/commit-graph.c: UNLEAK variables + commit-graph: clean up leaked memory during write Code clean-up. * jc/how-to-document-api (2018-09-29) 1 commit (merged to 'next' on 2018-10-12 at 7c9bd82285) + CodingGuidelines: document the API in *.h files Doc update. * jt/avoid-ls-refs (2018-10-07) 4 commits (merged to 'next' on 2018-10-12 at 5775aabbc1) + fetch: do not list refs if fetching only hashes + transport: list refs before fetch if necessary + transport: do not list refs if possible + transport: allow skipping of ref listing Over some transports, fetching objects with an exact commit object name can be done without first seeing the ref advertisements. The code has been optimized to exploit this. * jt/cache-tree-allow-missing-object-in-partial-clone (2018-10-10) 1 commit (merged to 'next' on 2018-10-12 at 152ad8e336) + cache-tree: skip some blob checks in partial clone In a partial clone that will lazily be hydrated from the originating repository, we generally want to avoid "does this object exist (locally)?" on objects that we deliberately omitted when we created the clone. The cache-tree codepath (which is used to write a tree object out of the index) however insisted that the object exists, even for paths that are outside of the partial checkout area. The code has been updated to avoid such a check. * jt/fetch-tips-in-partial-clone (2018-09-21) 2 commits (merged to 'next' on 2018-10-12 at 521b3fb44d) + fetch: in partial clone, check presence of targets + connected: document connectivity in partial clones "git fetch $repo $object" in a partial clone did not correctly fetch the asked-for object that is referenced by an object in promisor packfile, which has been fixed. * jt/non-blob-lazy-fetch (2018-10-04) 2 commits (merged to 'next' on 2018-10-12 at 7466c6bd7d) + fetch-pack: exclude blobs when lazy-fetching trees + fetch-pack: avoid object flags if no_dependents A partial clone that is configured to lazily fetch missing objects will on-demand issue a "git fetch" request to the originating repository to fill not-yet-obtained objects. The request has been optimized for requesting a tree object (and not the leaf blob objects contained in it) by telling the originating repository that no blobs are needed. * nd/complete-fetch-multiple-args (2018-09-21) 1 commit (merged to 'next' on 2018-10-10 at f78e14123c) + completion: support "git fetch --multiple" Teach bash completion that "git fetch --multiple" only takes remote names as arguments and no refspecs. * nd/help-commands-verbose-by-default (2018-10-03) 1 commit (merged to 'next' on 2018-10-12 at 32de8f53e0) + help -a: improve and make --verbose default "git help -a" and "git
Re: [PATCH 1/1] commit-reach: fix first-parent heuristic
"Derrick Stolee via GitGitGadget" writes: > We can also re-run the performance tests from commit 4fbcca4e > "commit-reach: make can_all_from_reach... linear". > > Performance was measured on the Linux repository using > 'test-tool reach can_all_from_reach'. The input included rows seeded by > tag values. The "small" case included X-rows as v4.[0-9]* and Y-rows as > v3.[0-9]*. This mimics a (very large) fetch that says "I have all major > v3 releases and want all major v4 releases." The "large" case included > X-rows as "v4.*" and Y-rows as "v3.*". This adds all release-candidate > tags to the set, which does not greatly increase the number of objects > that are considered, but does increase the number of 'from' commits, > demonstrating the quadratic nature of the previous code. These micro-benchmarks are interesting, but we should also remember to describe the impact of the bug being fixed in the larger picture. What end-user visible operations are affected? Computing merge-base? Finding if a push fast-forwards? Something else?
[PATCH] receive: denyCurrentBranch=updateinstead should not blindly update
The handling of receive.denyCurrentBranch=updateInstead was added to
a switch statement that handles other values of the variable, but
all the other case arms only checked a condition to reject the
attempted push, or let later logic in the same function to still
intervene, so that a push that does not fast-forward (which is
checked after the switch statement in question) is still rejected.
But the handling of updateInstead incorrectly took immediate effect,
without giving other checks a chance to intervene.
Instead of calling update_worktree() that causes the side effect
immediately, just note the fact that we will need to call the
funciton later, and first give other checks chance to reject the
request. After the update-hook gets a chance to reject the push
(which happens as the last step in a series of checks), call
update_worktree() when we earlier detected the need to.
Reported-by: Rajesh Madamanchi
Signed-off-by: Junio C Hamano
---
builtin/receive-pack.c | 12 +---
t/t5516-fetch-push.sh | 8 +++-
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 95740f4f0e..79ee320948 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1026,6 +1026,7 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
const char *ret;
struct object_id *old_oid = >old_oid;
struct object_id *new_oid = >new_oid;
+ int do_update_worktree = 0;
/* only refs/... are allowed */
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -1051,9 +1052,8 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
refuse_unconfigured_deny();
return "branch is currently checked out";
case DENY_UPDATE_INSTEAD:
- ret = update_worktree(new_oid->hash);
- if (ret)
- return ret;
+ /* pass -- let other checks intervene first */
+ do_update_worktree = 1;
break;
}
}
@@ -1118,6 +1118,12 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
return "hook declined";
}
+ if (do_update_worktree) {
+ ret = update_worktree(new_oid->hash);
+ if (ret)
+ return ret;
+ }
+
if (is_null_oid(new_oid)) {
struct strbuf err = STRBUF_INIT;
if (!parse_object(the_repository, old_oid)) {
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 7a8f56db53..7316365a24 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1577,7 +1577,13 @@ test_expect_success 'receive.denyCurrentBranch =
updateInstead' '
test $(git -C .. rev-parse master) = $(git rev-parse HEAD) &&
git diff --quiet &&
git diff --cached --quiet
- )
+ ) &&
+
+ # (6) updateInstead intervened by fast-forward check
+ test_must_fail git push void master^:master &&
+ test $(git -C void rev-parse HEAD) = $(git rev-parse master) &&
+ git -C void diff --quiet &&
+ git -C void diff --cached --quiet
'
test_expect_success 'updateInstead with push-to-checkout hook' '
--
2.19.1-450-ga4b8ab5363
[PATCH v3] fetch: replace string-list used as a look-up table with a hashmap
In find_non_local_tags() helper function (used to implement the
"follow tags"), we use string_list_has_string() on two string lists
as a way to see if a refname has already been processed, etc.
All this code predates more modern in-core lookup API like hashmap;
replace them with two hashmaps and one string list---the hashmaps
are used for look-ups and the string list is to keep the order of
items in the returned result stable (which is the only single thing
hashmap does worse than lookups on string-list).
Similarly, get_ref_map() uses another string-list as a look-up table
for existing refs. Replace it with a hashmap.
Signed-off-by: Junio C Hamano
---
* This converts another string-list user in the same file. For now
I am done with this topic; I'm willing to fix bugs in this patch,
but do not intend to spend time killing more string-lists used as
look-up tables.
builtin/fetch.c | 152 +---
1 file changed, 106 insertions(+), 46 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 0696abfc2a..0f8e333022 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -223,18 +223,6 @@ static void add_merge_config(struct ref **head,
}
}
-static int add_existing(const char *refname, const struct object_id *oid,
- int flag, void *cbdata)
-{
- struct string_list *list = (struct string_list *)cbdata;
- struct string_list_item *item = string_list_insert(list, refname);
- struct object_id *old_oid = xmalloc(sizeof(*old_oid));
-
- oidcpy(old_oid, oid);
- item->util = old_oid;
- return 0;
-}
-
static int will_fetch(struct ref **head, const unsigned char *sha1)
{
struct ref *rm = *head;
@@ -246,16 +234,76 @@ static int will_fetch(struct ref **head, const unsigned
char *sha1)
return 0;
}
+struct refname_hash_entry {
+ struct hashmap_entry ent; /* must be the first member */
+ struct object_id oid;
+ char refname[FLEX_ARRAY];
+};
+
+static int refname_hash_entry_cmp(const void *hashmap_cmp_fn_data,
+ const void *e1_,
+ const void *e2_,
+ const void *keydata)
+{
+ const struct refname_hash_entry *e1 = e1_;
+ const struct refname_hash_entry *e2 = e2_;
+
+ return strcmp(e1->refname, keydata ? keydata : e2->refname);
+}
+
+static struct refname_hash_entry *refname_hash_add(struct hashmap *map,
+ const char *refname,
+ const struct object_id *oid)
+{
+ struct refname_hash_entry *ent;
+ size_t len = strlen(refname);
+
+ FLEX_ALLOC_MEM(ent, refname, refname, len);
+ hashmap_entry_init(ent, memhash(refname, len));
+ oidcpy(>oid, oid);
+ hashmap_add(map, ent);
+ return ent;
+}
+
+static int add_one_refname(const char *refname,
+ const struct object_id *oid,
+ int flag, void *cbdata)
+{
+ struct hashmap *refname_map = cbdata;
+
+ (void) refname_hash_add(refname_map, refname, oid);
+ return 0;
+}
+
+static void refname_hash_init(struct hashmap *map)
+{
+ hashmap_init(map, refname_hash_entry_cmp, NULL, 0);
+}
+
+static int refname_hash_exists(struct hashmap *map, const char *refname)
+{
+ struct hashmap_entry key;
+ size_t len = strlen(refname);
+ hashmap_entry_init(, memhash(refname, len));
+
+ return !!hashmap_get(map, , refname);
+}
+
static void find_non_local_tags(const struct ref *refs,
struct ref **head,
struct ref ***tail)
{
- struct string_list existing_refs = STRING_LIST_INIT_DUP;
- struct string_list remote_refs = STRING_LIST_INIT_NODUP;
+ struct hashmap existing_refs;
+ struct hashmap remote_refs;
+ struct string_list remote_refs_list = STRING_LIST_INIT_NODUP;
+ struct string_list_item *remote_ref_item;
const struct ref *ref;
- struct string_list_item *item = NULL;
+ struct refname_hash_entry *item = NULL;
- for_each_ref(add_existing, _refs);
+ refname_hash_init(_refs);
+ refname_hash_init(_refs);
+
+ for_each_ref(add_one_refname, _refs);
for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
@@ -271,10 +319,10 @@ static void find_non_local_tags(const struct ref *refs,
!has_object_file_with_flags(>old_oid,
OBJECT_INFO_QUICK) &&
!will_fetch(head, ref->old_oid.hash) &&
- !has_sha1_file_with_flags(item->util,
+
Re: receive.denyCurrentBranch=updateInstead updates working tree even when receive.denyNonFastForwards rejects push
Junio C Hamano writes:
> Rajesh Madamanchi writes:
>
>> Hi, I am looking to report the below behavior when seems incorrect to
>> me when receive.denyCurrentBranch is set to updateInstead and
>> receive.denyNonFastForwards is set to true.
>
> It seems that we took a lazy but incorrect route while adding the
> DENY_UPDATE_INSTEAD hack to builtin/receive-pack.c::update() and new
> code went to a wrong place in a series of checks. Everythng else in
> the same switch() statement either refuses or just decides to let
> later step to update without taking actual action, so that later
> checks such as "the new tip commit must have been transferred", "the
> new tip must be a fast-forward of the old tip", etc., but the one
> for DENY_UPDATE_INSTEAD immediately calls update_worktree() there.
> It should be changed to decide to later call the function when
> everybody else in the series of checks agrees that it is OK to let
> this push accepted, and then the actual call is made somewhere near
> where we call run_update_hook(), probably after the hook says it is
> OK to update.
So here is a lunch-time hack that is not even compile tested but
illustrates the idea outlined above. We'd need to add tests to
protect the fix from future breakages (if the fix is correct, that
is, which I do not quite know---but it feels right ;-).
builtin/receive-pack.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7f089be11e..4bf316dbba 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1050,9 +1050,7 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
refuse_unconfigured_deny();
return "branch is currently checked out";
case DENY_UPDATE_INSTEAD:
- ret = update_worktree(new_oid->hash);
- if (ret)
- return ret;
+ /* pass -- let other checks intervene first */
break;
}
}
@@ -1117,6 +1115,12 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
return "hook declined";
}
+ if (deny_current_branch == DENY_UPDATE_INSTEAD) {
+ ret = update_worktree(new_oid->hash);
+ if (ret)
+ return ret;
+ }
+
if (is_null_oid(new_oid)) {
struct strbuf err = STRBUF_INIT;
if (!parse_object(the_repository, old_oid)) {
Re: [PATCH v2 0/3] Clear flags before each v2 request
Jonathan Tan writes:
> Jonathan Tan (3):
> upload-pack: make have_obj not global
> upload-pack: make want_obj not global
> upload-pack: clear flags before each v2 request
It took a bit of time why 2/3 did not apply cleanly but it turns out
this is based on a slightly older tip of 'master' (but still ahead
of 'maint'), that lacks 829a3215 ("commit-graph: close_commit_graph
before shallow walk", 2018-08-20). Applying and merging it to make
it up-to-date with the tip of 'master' went smoothly once I figured
it out.
The first two clean-up patches are probably overdue and worth doing
regardless of the bugfix. Nicely done.
The first two steps use static objects local to a transport method
to "preserve the existing behaviour", and because this codepath
happens to want a clean slate every time it gets called, the third
step manages to lose it, which is a nice progression. But it makes
me wonder if it also hints that there may be a need to invent a
state object that is passed around from the transport layer across
requests, if we want to fulfill a request by calling multiple
transport methods in the future. In any case, there is no immediate
need to address this comment ;-).
Will replace. Thanks.
Re: [PATCH v4 9/9] Documentation/config: add odb..promisorRemote
Jonathan Nieder writes: > Junio C Hamano wrote: > ... >> It is a good idea to implicitly include the promisor-remote to the >> set of secondary places to consult to help existing versions of Git, >> but once the repository starts fetching incomplete subgraphs and >> adding new object.missingobjectremote [*1*], these versions of Git >> will stop working correctly, so I am not sure if it is all that >> useful approach for compatibility in practice. > > Can you spell this out for me more? Do you mean that a remote from > this list might make a promise that the original partialClone remote > can't keep? It was my failed attempt to demonstrate that I understood what was being discussed by rephrasing JTan's Or allow extensions.partialClone= wherein is not in the missingObjectRemote, in which case is tried first, so that we don't have to reject some configurations.
Re: [PATCH 0/1] commit-reach: fix first-parent heuristic
"Derrick Stolee via GitGitGadget" writes: > I originally reported this fix [1] after playing around with the trace2 > series for measuring performance. Since trace2 isn't merging quickly, I > pulled the performance fix patch out and am sending it on its own. The only > difference here is that we don't have the tracing to verify the performance > fix in the test script. Thanks for sending this separately. What's the current status of the tracing patch series, by the way?
Re: [PATCH] multi-pack-index: avoid dead store for struct progress
Carlo Marcelo Arenas Belón writes:
> it is initialized unconditionally by a call to start_progress
> below.
>
> Signed-off-by: Carlo Marcelo Arenas Belón
> ---
> midx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/midx.c b/midx.c
> index ea2f3ffe2e..4fac0cd08a 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -941,7 +941,7 @@ static void midx_report(const char *fmt, ...)
> int verify_midx_file(const char *object_dir)
> {
> uint32_t i;
> - struct progress *progress = NULL;
> + struct progress *progress;
> struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);
> verify_midx_error = 0;
Both changes make sense. It's kind of surprising that this still
matters, though; I would have expected a compiler would notice.
Re: [PATCH v2 02/13] ci/lib.sh: encapsulate Travis-specific things
SZEDER Gábor writes: > On Mon, Oct 15, 2018 at 03:12:00AM -0700, Johannes Schindelin via > GitGitGadget wrote: >> diff --git a/ci/lib.sh b/ci/lib.sh >> index 06970f7213..8532555b4e 100755 >> --- a/ci/lib.sh >> +++ b/ci/lib.sh >> @@ -1,5 +1,26 @@ >> # Library of functions shared by all CI scripts >> >> +if test true = "$TRAVIS" >> +then >> +... >> +export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save" >> +export GIT_TEST_OPTS="--verbose-log -x --immediate" >> +fi > > Please set all these variables ... Do you mean "VAR=VAL; export VAR" is kosher, "export VAR=VAL" is not? >> @@ -81,7 +102,6 @@ check_unignored_build_artifacts () >> # and installing dependencies. >> set -ex > > ... after we turn on 'set -x', so the variables' values will be > visible in the logs. Ah, no, you didn't. Although I think both are valid points, I think ci/lib.sh is expected to be used only inside a more predictable environment (e.g. we know the shell used is not a random POSIX shell but one that is happy with "export VAR=VAL"), so it should be OK. Showing the values of these variables in the log may still be good idea. > (Or move this 'set -ex' to the beginning of the script? Then we > could perhaps avoid similar issues in the future.) Sure (provided that it is an issue to begin with---if we are interested in the value of TRAVIS_BRANCH, for example, being able to see it only because "CI_BRANCH=$TRAVIS_BRANCH" assignment is made feels a bit sideways---we'd be better off explicitly logging anything we are interested in in the longer term, no?).
Re: [PATCH v1 1/1] index_bulk_checkin(): Take off_t, not size_t
tbo...@web.de writes:
> bulk-checkin.c | 4 ++--
> bulk-checkin.h | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
If you lost SP in your editor, then it is OK but if format-patch
lost it for some reason, plasee tell me as we need to find the bug.
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 409ecb566b..2631e82d6c 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -189,7 +189,7 @@ static void prepare_to_stream(struct bulk_checkin_state
> *state,
>
> static int deflate_to_pack(struct bulk_checkin_state *state,
> struct object_id *result_oid,
> -int fd, size_t size,
> +int fd, off_t size,
The size is once casted to uintmax_t for recording in the object
header (which is fine), and then passed to stream_to_pack(), which
still takes, and more importantly, does comparisons and chunking in,
size_t after this patch. Without xsize_t() around size passed in
the call to stream_to_pack(), you may silently be truncating off_t
down to size_t in this function.
> @@ -258,7 +258,7 @@ static int deflate_to_pack(struct bulk_checkin_state
> *state,
> }
>
> int index_bulk_checkin(struct object_id *oid,
> -int fd, size_t size, enum object_type type,
> +int fd, off_t size, enum object_type type,
> const char *path, unsigned flags)
> {
> int status = deflate_to_pack(, oid, fd, size, type,
This one is a thin wrapper around deflate_to_pack() above.
Its sole caller is sha1-file.c::index_stream() and takes size_t from
its callers, and passes size_t to index_bulk_checkin().
The sole caller of index_stream(), sha1-file.c::index_fd(), wants to
pass st->st_size, and it uses xsize_t() because index_stream() and
callchain underneath currently take size_t. You want that callchain
to take off_t with this patch.
The whole purpose of stream_to_pack() is to take potentially large
input from the file on the filesystem, chop that into manageable
chunks and feed the underlying hashing and deflating machinery that
takes possibly smaller integer types to represent the sizes of data
they take in a single call, so once that function is taught to take
ofs_t I think you can say you converted the entire callchain from
index_fd() down.
So, this looks like a good starting step, but I suspect it needs one
more level of digging for it to become truly useful.
Re: [PATCH] diff: don't attempt to strip prefix from absolute Windows paths
Johannes Sixt writes:
> Use is_absolute_path() to detect Windows style absolute paths.
When cd676a51 ("diff --relative: output paths as relative to the
current subdirectory", 2008-02-12) was done, neither "is_dir_sep()"
nor "has_dos_drive_prefix()" existed---the latter had to wait until
25fe217b ("Windows: Treat Windows style path names.", 2008-03-05),
but we didn't notice that the above code needs to use the Windows
aware is_absolute_path() when we applied the change.
> One might wonder whether the check for a directory separator that
> is visible in the patch context should be changed from == '/' to
> is_dir_sep() or not. It turns out not to be necessary. That code
> only ever investigates paths that have undergone pathspec
> normalization, after which there are only forward slashes even on
> Windows.
Thanks for carefully explaining.
> static void strip_prefix(int prefix_length, const char **namep, const char
> **otherp)
> {
> /* Strip the prefix but do not molest /dev/null and absolute paths */
> - if (*namep && **namep != '/') {
> + if (*namep && !is_absolute_path(*namep)) {
> *namep += prefix_length;
> if (**namep == '/')
> ++*namep;
> }
> - if (*otherp && **otherp != '/') {
> + if (*otherp && !is_absolute_path(*otherp)) {
> *otherp += prefix_length;
> if (**otherp == '/')
> ++*otherp;
When I read the initial report and guessed the root cause without
looking at the code, I didn't expect the problematic area to be this
isolated and the solution to be this simple.
Nicely done.
Re: [PATCH] config.mak.dev: enable -Wunused-function
Ramsay Jones writes:
> I haven't looked too deeply, but this seems to be caused by
> Junio's commit 42c89ea70a ("SQUASH??? - convert the other user of
> string-list as db", 2018-10-17) which removes a call to the
> add_existing() function - the subject of the warning.
That is very understandable and it is immediately obvious to me that
the unused-function warning is being useful there ;-)
Re: [PATCH 1/3] t6501: use --quiet when testing gc stderr
Derrick Stolee writes: > This code from builtin/gc.c makes it look like we are doing that: > > if (gc_write_commit_graph) > write_commit_graph_reachable(get_object_directory(), 0, > !quiet && !daemonized); > > But really, daemonized is only for when running in the background. But that is something we can easily fix, no? "git grep isatty" tells you that we use isatty(2) to decide if we want to go verbose or show progress. Another frequent use of isatty in our codebase of course is to use of a pager and columnar output formats.
Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet
Ben Peart writes: > Note the status command after the reset doesn't really change as it > still must lstat() every file (the 0.02 difference is well within the > variability of run to run differences). Of course, it would not make an iota of difference, whether reset refreshes the cached stat index fully, to the cost of later lstat(). What the refreshing saves is having to scan the contents to find that the file is unchanged at runtime. If your lstat() is not significantly faster than opening and scanning the file, the optimization based on the cached-stat information becomes moot. In a working tree full of unmodified files, stale cached-stat info in the index will cause us to compare the contents and waste a lot of time, and that is what refreshing avoids. If the "status" in your test sequence do not have to do that (e.g. the cached-stat information is already up-to-date and there is no point running refresh in reset), then I'd expect no difference between these two tests. > To move this forward, here is what I propose: > > 1) If the '--quiet' flag is passed, we silently take advantage of the > fact we can avoid having to do an "extra" lstat() of every file and > scope the refresh_index() call to those paths that we know have > changed. That's pretty much what the patch under discussion does. > 2) I can remove the note in the documentation of --quiet which I only > added to facilitate discoverability. Quite honestly, I am not sure if this (meaning #1 above) alone need to be even discoverable. Those who want --quiet output would use it, those who want to be told which paths are modified would not, and those who want to quickly be told which paths are modified would not be helped by the limited refresh anyway, so "with --quiet you can make it go faster" would not help anybody. > 3) I can also edit the documentation for reset.quietDefault (maybe I > should rename that to "reset.quiet"?) so that it does not discuss the > potential performance impact. I think reset.quiet (or reset.verbosity) is a good thing to have regardless.
Re: receive.denyCurrentBranch=updateInstead updates working tree even when receive.denyNonFastForwards rejects push
Rajesh Madamanchi writes: > Hi, I am looking to report the below behavior when seems incorrect to > me when receive.denyCurrentBranch is set to updateInstead and > receive.denyNonFastForwards is set to true. It seems that we took a lazy but incorrect route while adding the DENY_UPDATE_INSTEAD hack to builtin/receive-pack.c::update() and new code went to a wrong place in a series of checks. Everythng else in the same switch() statement either refuses or just decides to let later step to update without taking actual action, so that later checks such as "the new tip commit must have been transferred", "the new tip must be a fast-forward of the old tip", etc., but the one for DENY_UPDATE_INSTEAD immediately calls update_worktree() there. It should be changed to decide to later call the function when everybody else in the series of checks agrees that it is OK to let this push accepted, and then the actual call is made somewhere near where we call run_update_hook(), probably after the hook says it is OK to update.
Re: [PATCH v4 9/9] Documentation/config: add odb..promisorRemote
Jonathan Tan writes: >> [object] >> missingObjectRemote = local-cache-remote >> missingObjectRemote = origin >> > In the presence of missingObjectRemote, old versions of Git, when lazily > fetching, would only know to try the extensions.partialClone remote. But > this is safe because existing data wouldn't be clobbered (since we're > not using ideas like adding meaning to the contents of the .promisor > file). Also, other things like fsck and gc still work. It is a good idea to implicitly include the promisor-remote to the set of secondary places to consult to help existing versions of Git, but once the repository starts fetching incomplete subgraphs and adding new object.missingobjectremote [*1*], these versions of Git will stop working correctly, so I am not sure if it is all that useful approach for compatibility in practice. [Footnote] *1* That name with two "object" in it sounds horrible. I think the same keyname in 'core' section may sit better (this feature sounds more 'core' than other cruft that crept into 'core' section over time). Or "odb.remoteAlternate" (as opposed to object/info/alternates that are local alternates), perhaps.
Re: On overriding make variables from the environment...
SZEDER Gábor writes: > So, then it's either 'config.mak', or passing a 'CC=$CC' argument to > _all_ make commands, including those that are not supposed to build > anything, but only run the tests. I find the latter aesthetically not > particularly pleasing. The config.mak file is available for individual builder-testers to customize their build, and in the context of this discussion, I think the CI builder is just one particular individual who happens to be non human. If it is easy to throw suitable settings from within the CI configuration .yaml files into config.mak, I'd think that is exactly how the mechanism was invented to be used, so...
Re: [RFC] revision: Add --sticky-default option
Andreas Gruenbacher writes: >> > # is --stdin a selector, too? >> > branches | git log --stdin --not origin/master > > Yes, it's a positive selector (since --not doesn't apply to --stdin). But you should be able to do printf "%s\n" ^maint master | git rev-list --stdin Replace the second one with ^master and now you have nothing but negative. So, no, the line is much blurrier than you would think.
Re: [PATCH 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip
Stefan Beller writes:
> This is based on ao/submodule-wo-gitmodules-checked-out.
>
> This resends origin/sb/submodule-recursive-fetch-gets-the-tip, resolving
> the issues pointed out via
> origin/xxx/sb-submodule-recursive-fetch-gets-the-tip-in-pu
> by basing this series on origin/ao/submodule-wo-gitmodules-checked-out
Applying this round to the result of merging ao/submodule-* to
'master' requires this to work, it seems, as you've introduced a
call to repo-init thing in the meantime with another topic.
Subject: [PATCH] fixup! repository: repo_submodule_init to take a submodule
struct
---
builtin/submodule--helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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", )) {
--
2.19.1-450-ga4b8ab5363
Re: [RFC] revision: Add --sticky-default option
Jeff King writes:
> Just to play devil's advocate, how about this:
>
> git log --branches=jk/* --not origin/master
>
> Right now that shows nothing if there are no matching branches. But I
> think under the proposed behavior, it would start showing HEAD, which
> seems counter-intuitive.
>
> Or are we going to count any positive selector as a positive ref, even
> if it matches nothing?
That sounds like an intuitive behaviour of the command, but I may
change my mind when I look at other examples.
When viewing that "--branches=jk/*" example in isolation, yes, these
positive selectors that could produce positive revs should defeat
the --default, especially when it is built-in (like "log"). When
given by the user, I am not sure. With something like this:
git rev-list --default=HEAD --branches=jk/* ^master
clearly the user anticipates that jk/* may or may not produce any
positive refs; otherwise there is no point specifying the default.
But anyway...
> I could buy that, though it means that the
> command above is subtly different from one or both of:
>
> branches() {
> git for-each-ref --format='%(refname)' refs/heads/jk/*
> }
>
> # is --stdin a selector, too?
> branches | git log --stdin --not origin/master
>
> # here we have no idea that the user did a query and must show HEAD
> git log $(branches) --not origin/master
OK, scrap that---just as I predicted a few minutes ago, I now think
that "do we have a positive selector that can produce zero or more
result?" is an ill-defined question X-<.
Thanks for a doze of sanity.
Re: [PATCH 1/3] ref-filter: free memory from used_atom
Jeff King writes: > Presumably it came from the manual comment-style fixup. Wow, that was embarrassing. Thanks for catching it. > > With that fix, the tests run fine for me under ASan/UBSan (with the > exception of t5310, but that's fixed already in a parallel topic). > > -Peff
Re: [PATCH v2 2/2] merge-recursive: avoid showing conflicts with merge branch before HEAD
Elijah Newren writes:
> @@ -1283,6 +1302,18 @@ static int merge_mode_and_contents(struct
> merge_options *o,
> const char *branch2,
> struct merge_file_info *result)
> {
> + if (o->branch1 != branch1) {
> + /*
> + * It's weird getting a reverse merge with HEAD on the bottom
> + * side of the conflict markers and the other branch on the
> + * top. Fix that.
> + */
> + return merge_mode_and_contents(o, one, b, a,
> +filename,
> +branch2, branch1,
> +extra_marker_size, result);
> + }
Will queue with the following squashed in.
Thanks.
merge-recursive.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 16980db7f9..73b5710386 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1310,8 +1310,7 @@ static int merge_mode_and_contents(struct merge_options
*o,
*/
return merge_mode_and_contents(o, one, b, a,
filename,
- branch2, branch1,
- extra_marker_size, result);
+ branch2, branch1, result);
}
result->merge = 0;
