* Re: Protecting old temporary objects being reused from concurrent "git gc"?
From: Jeff King @ 2016-11-16 8:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matt McCutchen, git
In-Reply-To: <xmqqk2c4tsv4.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 15, 2016 at 12:01:35PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I suspect the issue is that read-tree populates the cache-tree index
> > extension, and then write-tree omits the object write before it even
> > gets to write_sha1_file(). The solution is that it should probably be
> > calling one of the freshen() functions (possibly just replacing
> > has_sha1_file() with check_and_freshen(), but I haven't looked).
>
> I think the final writing always happens via write_sha1_file(), but
> an earlier cache-tree update that says "if we have a tree object
> already, then use it, otherwise even though we know the object name
> for this subtree, do not record it in the cache-tree" codepath may
> decide to record the subtree's sha1 without refreshing the referent.
>
> A fix may look like this.
Yeah, that's along the lines I was expecting, though I'm not familiar
enough with cache-tree to say whether it's sufficient. I notice there is
a return very early on in update_one() when has_sha1_file() matches, and
it seems like that would trigger in some interesting cases, too.
-Peff
^ permalink raw reply
* Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C
From: Pranit Bauva @ 2016-11-16 17:09 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <098cb39e-3c92-df56-1dc9-c529df817262@gmx.net>
Hey Stephan,
On Wed, Nov 16, 2016 at 4:49 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 6a5878c..1d3e17f 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -24,6 +27,8 @@ static const char * const git_bisect_helper_usage[] = {
>> N_("git bisect--helper --bisect-check-and-set-terms <command> <TERM_GOOD> <TERM_BAD>"),
>> N_("git bisect--helper --bisect-next-check [<term>] <TERM_GOOD> <TERM_BAD"),
>> N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
>> + N_("git bisect--helper --bisect start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
>> + "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
>
> Typo: "--bisect start" with space instead of "-"
>
>> @@ -403,6 +408,205 @@ static int bisect_terms(struct bisect_terms *terms, const char **argv, int argc)
>> return 0;
>> }
>>
>> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
>> + const char **argv, int argc)
>> +{
>> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
>> + int flags, pathspec_pos, retval = 0;
>> + struct string_list revs = STRING_LIST_INIT_DUP;
>> + struct string_list states = STRING_LIST_INIT_DUP;
>> + struct strbuf start_head = STRBUF_INIT;
>> + struct strbuf bisect_names = STRBUF_INIT;
>> + struct strbuf orig_args = STRBUF_INIT;
>> + const char *head;
>> + unsigned char sha1[20];
>> + FILE *fp = NULL;
>> + struct object_id oid;
>> +
>> + if (is_bare_repository())
>> + no_checkout = 1;
>> +
>> + for (i = 0; i < argc; i++) {
>> + if (!strcmp(argv[i], "--")) {
>> + has_double_dash = 1;
>> + break;
>> + }
>> + }
>> +
>> + for (i = 0; i < argc; i++) {
>> + const char *commit_id = xstrfmt("%s^{commit}", argv[i]);
>> + const char *arg = argv[i];
>> + if (!strcmp(argv[i], "--")) {
>> + has_double_dash = 1;
>
> This is without effect since has_double_dash is already set to 1 by the
> loop above. I think you can remove this line.
True. I will remove this line.
>> + break;
>> + } else if (!strcmp(arg, "--no-checkout")) {
>> + no_checkout = 1;
>> + } else if (!strcmp(arg, "--term-good") ||
>> + !strcmp(arg, "--term-old")) {
>> + must_write_terms = 1;
>> + terms->term_good = xstrdup(argv[++i]);
>> + } else if (skip_prefix(arg, "--term-good=", &arg)) {
>> + must_write_terms = 1;
>> + terms->term_good = xstrdup(arg);
>> + } else if (skip_prefix(arg, "--term-old=", &arg)) {
>> + must_write_terms = 1;
>> + terms->term_good = xstrdup(arg);
>
> I think you can join the last two branches:
>
> + } else if (skip_prefix(arg, "--term-good=", &arg) ||
> + skip_prefix(arg, "--term-old=", &arg)) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(arg);
>
>> + } else if (!strcmp(arg, "--term-bad") ||
>> + !strcmp(arg, "--term-new")) {
>> + must_write_terms = 1;
>> + terms->term_bad = xstrdup(argv[++i]);
>> + } else if (skip_prefix(arg, "--term-bad=", &arg)) {
>> + must_write_terms = 1;
>> + terms->term_bad = xstrdup(arg);
>> + } else if (skip_prefix(arg, "--term-new=", &arg)) {
>> + must_write_terms = 1;
>> + terms->term_good = xstrdup(arg);
>
> This has to be terms->term_bad = ...
My bad.
> Also, you can join the last two branches, again, ie,
Sure!
> + } else if (skip_prefix(arg, "--term-bad=", &arg) ||
> + skip_prefix(arg, "--term-new=", &arg)) {
> + must_write_terms = 1;
> + terms->term_bad = xstrdup(arg);
>
>> + } else if (starts_with(arg, "--") &&
>> + !one_of(arg, "--term-good", "--term-bad", NULL)) {
>> + die(_("unrecognised option: '%s'"), arg);
> [...]
>> + /*
>> + * Verify HEAD
>> + */
>> + head = resolve_ref_unsafe("HEAD", 0, sha1, &flags);
>> + if (!head)
>> + if (get_sha1("HEAD", sha1))
>> + die(_("Bad HEAD - I need a HEAD"));
>> +
>> + if (!is_empty_or_missing_file(git_path_bisect_start())) {
>
> You were so eager to re-use the comments from the shell script, but you
> forgot the "Check if we are bisecting." comment above this line ;-)
I will add it back again.
>> + /* Reset to the rev from where we started */
>> + strbuf_read_file(&start_head, git_path_bisect_start(), 0);
>> + strbuf_trim(&start_head);
>> + if (!no_checkout) {
>> + struct argv_array argv = ARGV_ARRAY_INIT;
> [...]
>> + if (must_write_terms)
>> + if (write_terms(terms->term_bad, terms->term_good)) {
>> + retval = -1;
>> + goto finish;
>> + }
>> +
>
> bisect_start() is a pretty big function.
> I think it can easily be decomposed in some smaller parts, for example,
> the following lines ...
>
>> + fp = fopen(git_path_bisect_log(), "a");
>> + if (!fp)
>> + return -1;
>> +
>> + if (fprintf(fp, "git bisect start") < 1) {
>> + retval = -1;
>> + goto finish;
>> + }
>> +
>> + sq_quote_argv(&orig_args, argv, 0);
>> + if (fprintf(fp, "%s", orig_args.buf) < 0) {
>> + retval = -1;
>> + goto finish;
>> + }
>> + if (fprintf(fp, "\n") < 1) {
>> + retval = -1;
>> + goto finish;
>> + }
>
> ... could be in a function like
>
> static int bisect_append_log(const char **argv)
> {
> FILE *fp = fopen(git_path_bisect_log(), "a");
> struct strbuf orig_args = STRBUF_INIT;
> if (!fp)
> return -1;
>
> if (fprintf(fp, "git bisect start") < 1) {
> retval = -1;
> goto finish;
> }
>
> sq_quote_argv(&orig_args, argv, 0);
> if (fprintf(fp, "%s", orig_args.buf) < 0 ||
> fprintf(fp, "\n") < 1) {
> retval = -1;
> goto finish;
> }
>
> finish:
> if (fp)
> fclose(fp);
> strbuf_release(&orig_args);
>
> return retval;
> }
>
> and then simply call
>
> retval = bisect_append_log(argv);
>
> in bisect_start()... (This is totally untested.)
I think this would be a better choice. Thanks!
> If you do not want that for some reason, you should at least fix
>
>> + if (!fp)
>> + return -1;
>
> to retval = 1; goto finish; such that the other lists and strings are
> released.
>
>> + goto finish;
>> +finish:
>
> The "goto finish" right above the "finish" label is unnecessary.
>
>> + if (fp)
>> + fclose(fp);
>> + string_list_clear(&revs, 0);
>> + string_list_clear(&states, 0);
>> + strbuf_release(&start_head);
>> + strbuf_release(&bisect_names);
>> + strbuf_release(&orig_args);
>> + return retval;
>> +}
>> +
>> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> {
>> enum {
>
> By the way, there are two spaghetti-ish ways to get rid of the
>
> retval = -1;
> goto finish;
>
> line pair:
>
> goto fail;
>
> and below the "return retval;" add
>
> fail:
> retval = -1;
> goto finish;
>
> and you can feel the touch of His Noodly Appendage. *scnr*
Nice little trick.
> The other way is to keep the "goto finish" I deemed unnecessary (right
> above the label), and expand it to:
>
> goto finish;
> fail:
> retval = -1;
> finish:
> ...
Regards,
Pranit Bauva
^ permalink raw reply
* RE: [PATCH 14/16] checkout: recurse into submodules if asked to
From: David Turner @ 2016-11-16 17:05 UTC (permalink / raw)
To: 'Stefan Beller'
Cc: git@vger.kernel.org, bmwill@google.com, gitster@pobox.com,
jrnieder@gmail.com, mogulguy10@gmail.com
In-Reply-To: <20161115230651.23953-15-sbeller@google.com>
Sorry, my previous message accidentally sent before I was done. One more comment:
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> +test_expect_failure '"checkout --recurse-submodules" needs -f to update
> modifed submodule content' '
> + echo modified >submodule/second.t &&
> + test_must_fail git checkout --recurse-submodules HEAD^ &&
> + test_must_fail git diff-files --quiet submodule &&
> + git diff-files --quiet file &&
> + git checkout --recurse-submodules -f HEAD^ &&
> + git diff-files --quiet &&
> + git diff-index --quiet --cached HEAD &&
> + git checkout --recurse-submodules -f master &&
> + git diff-files --quiet &&
> + git diff-index --quiet --cached HEAD
> +'
It might be worth adding some comments explaining why you expect these to fail.
^ permalink raw reply
* RE: [PATCH 14/16] checkout: recurse into submodules if asked to
From: David Turner @ 2016-11-16 17:03 UTC (permalink / raw)
To: 'Stefan Beller'
Cc: git@vger.kernel.org, bmwill@google.com, gitster@pobox.com,
jrnieder@gmail.com, mogulguy10@gmail.com
In-Reply-To: <20161115230651.23953-15-sbeller@google.com>
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
>
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index
> 79cdd34..e0773c6 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -634,7 +634,13 @@ test_submodule_forced_switch () {
>
> ########################## Modified submodule
> #########################
> # Updating a submodule sha1 doesn't update the submodule's work tree
> - test_expect_success "$command: modified submodule does not update
> submodule work tree" '
> + if test
> "$KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST" =
> 1
> + then
> + RESULT="failure"
> + else
> + RESULT="success"
> + fi
> + test_expect_$RESULT "$command: modified submodule does not update
> submodule work tree" '
Why does this break? I thought it was only if checkout is run with --recurse-submodules that anything should change?
> +test_expect_success 'dirty file file is not deleted' '
Duplicate 'file' in this test name.
> +# This is ok in theory, we just need to make sure # the garbage
> +collection doesn't eat the commit.
> +test_expect_success 'different commit prevents from deleting' '
This isn't a different commit -- it's a dirty index, right?
> +test_expect_failure '"checkout --recurse-submodules" does not care about
> untracked submodule content' '
> + echo untracked >submodule/untracked &&
> + git checkout --recurse-submodules master &&
> + git diff-files --quiet --ignore-submodules=untracked &&
> + git diff-index --quiet --cached HEAD &&
> + rm submodule/untracked
> +'
Use test_when_finished for cleanup.
> +test_expect_failure '"checkout --recurse-submodules" needs -f when
> submodule commit is not present (but does fail anyway)' '
> + git checkout --recurse-submodules -b bogus_commit master &&
> + git update-index --cacheinfo 160000
> 0123456789012345678901234567890123456789 submodule &&
> + BOGUS_TREE=$(git write-tree) &&
> + BOGUS_COMMIT=$(echo "bogus submodule commit" | git commit-tree
> $BOGUS_TREE) &&
> + git commit -m "bogus submodule commit" &&
> + git checkout --recurse-submodules -f master &&
> + test_must_fail git checkout --recurse-submodules bogus_commit &&
> + git diff-files --quiet &&
> + test_must_fail git checkout --recurse-submodules -f bogus_commit &&
> + test_must_fail git diff-files --quiet submodule &&
> + git diff-files --quiet file &&
> + git diff-index --quiet --cached HEAD &&
> + git checkout --recurse-submodules -f master '
> +KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST=1
> test_submodule_switch "git checkout"
>
> +KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST=
> test_submodule_forced_switch "git checkout -f"
>
> test_done
> --
> 2.10.1.469.g00a8914
^ permalink raw reply
* Re: [PATCH v15 04/27] bisect--helper: `bisect_clean_state` shell function in C
From: Pranit Bauva @ 2016-11-16 16:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stephan Beyer, Git List
In-Reply-To: <xmqq37istoay.fsf@gitster.mtv.corp.google.com>
Hey Junio,
On Wed, Nov 16, 2016 at 3:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Stephan Beyer <s-beyer@gmx.net> writes:
>
> >> +int bisect_clean_state(void)
> >> +{
> >> + int result = 0;
> >> +
> >> + /* There may be some refs packed during bisection */
> >> + struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
> >> + for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
> >> + string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
> >> + result = delete_refs(&refs_for_removal, REF_NODEREF);
> >> + refs_for_removal.strdup_strings = 1;
> >> + string_list_clear(&refs_for_removal, 0);
> >
> > Does it have advantages to populate a list (with duplicated strings),
> > hand it to delete_refs(), and clear the list (and strings), instead of
> > just doing a single delete_ref() (or whatever name the singular function
> > has) in the callback?
>
> Depending on ref backends, removing multiple refs may be a lot more
> efficient than calling a single ref removal for the same set of
> refs, and the comment upfront I think hints that the code was
> written in the way exactly with that in mind. Removing N refs from
> a packed refs file will involve a loop that runs N times, each
> iteration loading the file, locating an entry among possibly 100s of
> refs to remove, and then rewriting the file.
>
> Besides, it is bad taste to delete each individual item being
> iterated over in an interator in general, isn't it?
>
Not just that, deleting a ref inside for_each*() is illegal because it
builds some kind of index and that is spoiled if anything is deleted
in between. Thus it gives a seg fault. See this[1]. I did the same
mistake when making this patch and I was confused about that was
happening but then Michael Haggerty pointed this out[2].
[1]: https://github.com/git/git/blob/v2.11.0-rc1/refs.h#L183-L191
[2]: http://public-inbox.org/git/574D122F.7080608@alum.mit.edu/
Regards,
Pranit Bauva
^ permalink raw reply
* Re: merge --no-ff is NOT mentioned in help
From: Mike Rappazzo @ 2016-11-16 15:57 UTC (permalink / raw)
To: Vanderhoof, Tzadik; +Cc: git@vger.kernel.org
In-Reply-To: <2C8817BDA27E034F8E9A669458E375EF2BE689@APSWP0428.ms.ds.uhc.com>
(Please reply inline)
On Wed, Nov 16, 2016 at 10:48 AM, Vanderhoof, Tzadik
<tzadik.vanderhoof@optum360.com> wrote:
> I am running: git version 2.10.1.windows.1
>
> I typed: git merge -h
>
> and got:
>
> usage: git merge [<options>] [<commit>...]
> or: git merge [<options>] <msg> HEAD <commit>
> or: git merge --abort
>
> -n do not show a diffstat at the end of the merge
> --stat show a diffstat at the end of the merge
> --summary (synonym to --stat)
> --log[=<n>] add (at most <n>) entries from shortlog to merge commit message
> --squash create a single commit instead of doing a merge
> --commit perform a commit if the merge succeeds (default)
> -e, --edit edit message before committing
> --ff allow fast-forward (default)
> --ff-only abort if fast-forward is not possible
> --rerere-autoupdate update the index with reused conflict resolution if possible
> --verify-signatures verify that the named commit has a valid GPG signature
> -s, --strategy <strategy>
> merge strategy to use
> -X, --strategy-option <option=value>
> option for selected merge strategy
> -m, --message <message>
> merge commit message (for a non-fast-forward merge)
> -v, --verbose be more verbose
> -q, --quiet be more quiet
> --abort abort the current in-progress merge
> --allow-unrelated-histories
> allow merging unrelated histories
> --progress force progress reporting
> -S, --gpg-sign[=<key-id>]
> GPG sign commit
> --overwrite-ignore update ignored files (default)
>
> Notice there is NO mention of the "--no-ff" option
I understand. On my system I can reproduce this by providing a bad
argument to `git merge`. This is the output from the arg setup. For
"boolean" arguments (like '--ff'), there is an automatic counter
argument with "no-" in there ('--no-ff') to disable the option. Maybe
it would make sense to word the output to include both.
>
> -----Original Message-----
> From: Mike Rappazzo [mailto:rappazzo@gmail.com]
> Sent: Wednesday, November 16, 2016 7:37 AM
> To: Vanderhoof, Tzadik
> Cc: git@vger.kernel.org
> Subject: Re: merge --no-ff is NOT mentioned in help
>
> On Wed, Nov 16, 2016 at 10:16 AM, Vanderhoof, Tzadik <tzadik.vanderhoof@optum360.com> wrote:
>> When I do: "git merge -h" to get help, the option "--no-ff" is left out of the list of options.
>
> I am running git version 2.10.0, and running git merge --help contains these lines:
>
> --ff
> When the merge resolves as a fast-forward, only update the branch pointer, without creating a merge commit. This is the default behavior.
>
> --no-ff
> Create a merge commit even when the merge resolves as a fast-forward. This is the default behaviour when merging an annotated (and possibly signed) tag.
>
> --ff-only
> Refuse to merge and exit with a non-zero status unless the current HEAD is already up-to-date or the merge can be resolved as a fast-forward.
>
>
>
> This e-mail, including attachments, may include confidential and/or
> proprietary information, and may be used only by the person or entity
> to which it is addressed. If the reader of this e-mail is not the intended
> recipient or his or her authorized agent, the reader is hereby notified
> that any dissemination, distribution or copying of this e-mail is
> prohibited. If you have received this e-mail in error, please notify the
> sender by replying to this message and delete this e-mail immediately.
^ permalink raw reply
* RE: merge --no-ff is NOT mentioned in help
From: Vanderhoof, Tzadik @ 2016-11-16 15:48 UTC (permalink / raw)
To: Mike Rappazzo; +Cc: git@vger.kernel.org
In-Reply-To: <CANoM8SX91JAvJ6EAE6=wavPutUG4ZU1BY-A=5EobW=8zrdEcjw@mail.gmail.com>
I am running: git version 2.10.1.windows.1
I typed: git merge -h
and got:
usage: git merge [<options>] [<commit>...]
or: git merge [<options>] <msg> HEAD <commit>
or: git merge --abort
-n do not show a diffstat at the end of the merge
--stat show a diffstat at the end of the merge
--summary (synonym to --stat)
--log[=<n>] add (at most <n>) entries from shortlog to merge commit message
--squash create a single commit instead of doing a merge
--commit perform a commit if the merge succeeds (default)
-e, --edit edit message before committing
--ff allow fast-forward (default)
--ff-only abort if fast-forward is not possible
--rerere-autoupdate update the index with reused conflict resolution if possible
--verify-signatures verify that the named commit has a valid GPG signature
-s, --strategy <strategy>
merge strategy to use
-X, --strategy-option <option=value>
option for selected merge strategy
-m, --message <message>
merge commit message (for a non-fast-forward merge)
-v, --verbose be more verbose
-q, --quiet be more quiet
--abort abort the current in-progress merge
--allow-unrelated-histories
allow merging unrelated histories
--progress force progress reporting
-S, --gpg-sign[=<key-id>]
GPG sign commit
--overwrite-ignore update ignored files (default)
Notice there is NO mention of the "--no-ff" option
-----Original Message-----
From: Mike Rappazzo [mailto:rappazzo@gmail.com]
Sent: Wednesday, November 16, 2016 7:37 AM
To: Vanderhoof, Tzadik
Cc: git@vger.kernel.org
Subject: Re: merge --no-ff is NOT mentioned in help
On Wed, Nov 16, 2016 at 10:16 AM, Vanderhoof, Tzadik <tzadik.vanderhoof@optum360.com> wrote:
> When I do: "git merge -h" to get help, the option "--no-ff" is left out of the list of options.
I am running git version 2.10.0, and running git merge --help contains these lines:
--ff
When the merge resolves as a fast-forward, only update the branch pointer, without creating a merge commit. This is the default behavior.
--no-ff
Create a merge commit even when the merge resolves as a fast-forward. This is the default behaviour when merging an annotated (and possibly signed) tag.
--ff-only
Refuse to merge and exit with a non-zero status unless the current HEAD is already up-to-date or the merge can be resolved as a fast-forward.
This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or his or her authorized agent, the reader is hereby notified
that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.
^ permalink raw reply
* Re: merge --no-ff is NOT mentioned in help
From: Mike Rappazzo @ 2016-11-16 15:36 UTC (permalink / raw)
To: Vanderhoof, Tzadik; +Cc: git@vger.kernel.org
In-Reply-To: <2C8817BDA27E034F8E9A669458E375EF2BE63B@APSWP0428.ms.ds.uhc.com>
On Wed, Nov 16, 2016 at 10:16 AM, Vanderhoof, Tzadik
<tzadik.vanderhoof@optum360.com> wrote:
> When I do: "git merge -h" to get help, the option "--no-ff" is left out of the list of options.
I am running git version 2.10.0, and running git merge --help contains
these lines:
--ff
When the merge resolves as a fast-forward, only update the
branch pointer, without creating a merge commit. This is the default
behavior.
--no-ff
Create a merge commit even when the merge resolves as a
fast-forward. This is the default behaviour when merging an annotated
(and possibly signed) tag.
--ff-only
Refuse to merge and exit with a non-zero status unless the
current HEAD is already up-to-date or the merge can be resolved as a
fast-forward.
>
> This e-mail, including attachments, may include confidential and/or
> proprietary information, and may be used only by the person or entity
> to which it is addressed. If the reader of this e-mail is not the intended
> recipient or his or her authorized agent, the reader is hereby notified
> that any dissemination, distribution or copying of this e-mail is
> prohibited. If you have received this e-mail in error, please notify the
> sender by replying to this message and delete this e-mail immediately.
>
^ permalink raw reply
* Re: [PATCH v7 00/17] port branch.c to use ref-filter's printing options
From: Karthik Nayak @ 2016-11-16 15:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jacob Keller
In-Reply-To: <xmqqbmxgtqxv.fsf@gitster.mtv.corp.google.com>
On Wed, Nov 16, 2016 at 2:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> This is part of unification of the commands 'git tag -l, git branch -l
>> and git for-each-ref'. This ports over branch.c to use ref-filter's
>> printing options.
>>
>> Karthik Nayak (17):
>> ref-filter: implement %(if), %(then), and %(else) atoms
>> ref-filter: include reference to 'used_atom' within 'atom_value'
>> ref-filter: implement %(if:equals=<string>) and
>> %(if:notequals=<string>)
>> ref-filter: modify "%(objectname:short)" to take length
>> ref-filter: move get_head_description() from branch.c
>> ref-filter: introduce format_ref_array_item()
>> ref-filter: make %(upstream:track) prints "[gone]" for invalid
>> upstreams
>> ref-filter: add support for %(upstream:track,nobracket)
>> ref-filter: make "%(symref)" atom work with the ':short' modifier
>> ref-filter: introduce refname_atom_parser_internal()
>> ref-filter: introduce symref_atom_parser() and refname_atom_parser()
>> ref-filter: make remote_ref_atom_parser() use
>> refname_atom_parser_internal()
>> ref-filter: add `:dir` and `:base` options for ref printing atoms
>> ref-filter: allow porcelain to translate messages in the output
>> branch, tag: use porcelain output
>> branch: use ref-filter printing APIs
>> branch: implement '--format' option
>
> This is not a new issue, but --format='%(HEAD)' you stole from
> for-each-ref is broken when you are on an unborn branch, and the
> second patch from the tip makes "git branch" (no other args) on
> an unborn branch to segfault, when there are real branches that
> have commits.
>
> Something like this needs to go before that step.
>
> ref-filter.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 944671af5a..c71d7360d2 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1318,7 +1318,7 @@ static void populate_value(struct ref_array_item *ref)
>
> head = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
> sha1, NULL);
> - if (!strcmp(ref->refname, head))
> + if (head && !strcmp(ref->refname, head))
> v->s = "*";
> else
> v->s = " ";
>
>
Thanks, will add it in.
--
Regards,
Karthik Nayak
^ permalink raw reply
* merge --no-ff is NOT mentioned in help
From: Vanderhoof, Tzadik @ 2016-11-16 15:16 UTC (permalink / raw)
To: git@vger.kernel.org
When I do: "git merge -h" to get help, the option "--no-ff" is left out of the list of options.
This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or his or her authorized agent, the reader is hereby notified
that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.
^ permalink raw reply
* [PATCH v4 3/4] batch check whether submodule needs pushing into one call
From: Heiko Voigt @ 2016-11-16 15:11 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, git, Jeff King, Stefan Beller, Jens.Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <cover.1479308877.git.hvoigt@hvoigt.net>
We run a command for each sha1 change in a submodule. This is
unnecessary since we can simply batch all sha1's we want to check into
one command. Lets do it so we can speedup the check when many submodule
changes are in need of checking.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
submodule.c | 62 ++++++++++++++++++++++++++++++++-----------------------------
1 file changed, 33 insertions(+), 29 deletions(-)
diff --git a/submodule.c b/submodule.c
index 12ac1ea..11391fa 100644
--- a/submodule.c
+++ b/submodule.c
@@ -507,27 +507,49 @@ static int append_sha1_to_argv(const unsigned char sha1[20], void *data)
return 0;
}
-static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])
+static int check_has_commit(const unsigned char sha1[20], void *data)
{
- if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
+ int *has_commit = data;
+
+ if (!lookup_commit_reference(sha1))
+ *has_commit = 0;
+
+ return 0;
+}
+
+static int submodule_has_commits(const char *path, struct sha1_array *commits)
+{
+ int has_commit = 1;
+
+ if (add_submodule_odb(path))
+ return 0;
+
+ sha1_array_for_each_unique(commits, check_has_commit, &has_commit);
+ return has_commit;
+}
+
+static int submodule_needs_pushing(const char *path, struct sha1_array *commits)
+{
+ if (!submodule_has_commits(path, commits))
return 0;
if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
struct child_process cp = CHILD_PROCESS_INIT;
- const char *argv[] = {"rev-list", NULL, "--not", "--remotes", "-n", "1" , NULL};
struct strbuf buf = STRBUF_INIT;
int needs_pushing = 0;
- argv[1] = sha1_to_hex(sha1);
- cp.argv = argv;
+ argv_array_push(&cp.args, "rev-list");
+ sha1_array_for_each_unique(commits, append_sha1_to_argv, &cp.args);
+ argv_array_pushl(&cp.args, "--not", "--remotes", "-n", "1" , NULL);
+
prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
cp.dir = path;
if (start_command(&cp))
- die("Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s",
- sha1_to_hex(sha1), path);
+ die("Could not run 'git rev-list <commits> --not --remotes -n 1' command in submodule %s",
+ path);
if (strbuf_read(&buf, cp.out, 41))
needs_pushing = 1;
finish_command(&cp);
@@ -582,22 +604,6 @@ static void find_unpushed_submodule_commits(struct commit *commit,
diff_tree_combined_merge(commit, 1, &rev);
}
-struct collect_submodule_from_sha1s_data {
- char *submodule_path;
- struct string_list *needs_pushing;
-};
-
-static int collect_submodules_from_sha1s(const unsigned char sha1[20],
- void *data)
-{
- struct collect_submodule_from_sha1s_data *me = data;
-
- if (submodule_needs_pushing(me->submodule_path, sha1))
- string_list_insert(me->needs_pushing, me->submodule_path);
-
- return 0;
-}
-
static void free_submodules_sha1s(struct string_list *submodules)
{
struct string_list_item *item;
@@ -634,12 +640,10 @@ int find_unpushed_submodules(struct sha1_array *commits,
argv_array_clear(&argv);
for_each_string_list_item(submodule, &submodules) {
- struct collect_submodule_from_sha1s_data data;
- data.submodule_path = submodule->string;
- data.needs_pushing = needs_pushing;
- sha1_array_for_each_unique((struct sha1_array *) submodule->util,
- collect_submodules_from_sha1s,
- &data);
+ struct sha1_array *commits = (struct sha1_array *) submodule->util;
+
+ if (submodule_needs_pushing(submodule->string, commits))
+ string_list_insert(needs_pushing, submodule->string);
}
free_submodules_sha1s(&submodules);
--
2.10.1.386.gc503e45
^ permalink raw reply related
* [PATCH v4 2/4] serialize collection of refs that contain submodule changes
From: Heiko Voigt @ 2016-11-16 15:11 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, git, Jeff King, Stefan Beller, Jens.Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <cover.1479308877.git.hvoigt@hvoigt.net>
We are iterating over each pushed ref and want to check whether it
contains changes to submodules. Instead of immediately checking each ref
lets first collect them and then do the check for all of them in one
revision walk.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
submodule.c | 35 ++++++++++++++++++++---------------
submodule.h | 5 +++--
transport.c | 29 +++++++++++++++++++++--------
3 files changed, 44 insertions(+), 25 deletions(-)
diff --git a/submodule.c b/submodule.c
index b2908fe..12ac1ea 100644
--- a/submodule.c
+++ b/submodule.c
@@ -500,6 +500,13 @@ static int has_remote(const char *refname, const struct object_id *oid,
return 1;
}
+static int append_sha1_to_argv(const unsigned char sha1[20], void *data)
+{
+ struct argv_array *argv = data;
+ argv_array_push(argv, sha1_to_hex(sha1));
+ return 0;
+}
+
static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])
{
if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
@@ -599,25 +606,24 @@ static void free_submodules_sha1s(struct string_list *submodules)
string_list_clear(submodules, 1);
}
-int find_unpushed_submodules(unsigned char new_sha1[20],
+int find_unpushed_submodules(struct sha1_array *commits,
const char *remotes_name, struct string_list *needs_pushing)
{
struct rev_info rev;
struct commit *commit;
- const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
- int argc = ARRAY_SIZE(argv) - 1;
- char *sha1_copy;
struct string_list submodules = STRING_LIST_INIT_DUP;
struct string_list_item *submodule;
+ struct argv_array argv = ARGV_ARRAY_INIT;
- struct strbuf remotes_arg = STRBUF_INIT;
-
- strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
init_revisions(&rev, NULL);
- sha1_copy = xstrdup(sha1_to_hex(new_sha1));
- argv[1] = sha1_copy;
- argv[3] = remotes_arg.buf;
- setup_revisions(argc, argv, &rev, NULL);
+
+ /* argv.argv[0] will be ignored by setup_revisions */
+ argv_array_push(&argv, "find_unpushed_submodules");
+ sha1_array_for_each_unique(commits, append_sha1_to_argv, &argv);
+ argv_array_push(&argv, "--not");
+ argv_array_pushf(&argv, "--remotes=%s", remotes_name);
+
+ setup_revisions(argv.argc, argv.argv, &rev, NULL);
if (prepare_revision_walk(&rev))
die("revision walk setup failed");
@@ -625,8 +631,7 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
find_unpushed_submodule_commits(commit, &submodules);
reset_revision_walk();
- free(sha1_copy);
- strbuf_release(&remotes_arg);
+ argv_array_clear(&argv);
for_each_string_list_item(submodule, &submodules) {
struct collect_submodule_from_sha1s_data data;
@@ -663,12 +668,12 @@ static int push_submodule(const char *path)
return 1;
}
-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name)
+int push_unpushed_submodules(struct sha1_array *commits, const char *remotes_name)
{
int i, ret = 1;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
- if (!find_unpushed_submodules(new_sha1, remotes_name, &needs_pushing))
+ if (!find_unpushed_submodules(commits, remotes_name, &needs_pushing))
return 1;
for (i = 0; i < needs_pushing.nr; i++) {
diff --git a/submodule.h b/submodule.h
index d9e197a..9454806 100644
--- a/submodule.h
+++ b/submodule.h
@@ -3,6 +3,7 @@
struct diff_options;
struct argv_array;
+struct sha1_array;
enum {
RECURSE_SUBMODULES_CHECK = -4,
@@ -62,9 +63,9 @@ int submodule_uses_gitfile(const char *path);
int ok_to_remove_submodule(const char *path);
int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
const unsigned char a[20], const unsigned char b[20], int search);
-int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,
+int find_unpushed_submodules(struct sha1_array *commits, const char *remotes_name,
struct string_list *needs_pushing);
-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
+int push_unpushed_submodules(struct sha1_array *commits, const char *remotes_name);
void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
int parallel_submodules(void);
diff --git a/transport.c b/transport.c
index d57e8de..f482869 100644
--- a/transport.c
+++ b/transport.c
@@ -949,23 +949,36 @@ int transport_push(struct transport *transport,
if ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {
struct ref *ref = remote_refs;
+ struct sha1_array commits = SHA1_ARRAY_INIT;
+
for (; ref; ref = ref->next)
- if (!is_null_oid(&ref->new_oid) &&
- !push_unpushed_submodules(ref->new_oid.hash,
- transport->remote->name))
- die ("Failed to push all needed submodules!");
+ if (!is_null_oid(&ref->new_oid))
+ sha1_array_append(&commits, ref->new_oid.hash);
+
+ if (!push_unpushed_submodules(&commits, transport->remote->name)) {
+ sha1_array_clear(&commits);
+ die("Failed to push all needed submodules!");
+ }
+ sha1_array_clear(&commits);
}
if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {
struct ref *ref = remote_refs;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
+ struct sha1_array commits = SHA1_ARRAY_INIT;
for (; ref; ref = ref->next)
- if (!is_null_oid(&ref->new_oid) &&
- find_unpushed_submodules(ref->new_oid.hash,
- transport->remote->name, &needs_pushing))
- die_with_unpushed_submodules(&needs_pushing);
+ if (!is_null_oid(&ref->new_oid))
+ sha1_array_append(&commits, ref->new_oid.hash);
+
+ if (find_unpushed_submodules(&commits, transport->remote->name,
+ &needs_pushing)) {
+ sha1_array_clear(&commits);
+ die_with_unpushed_submodules(&needs_pushing);
+ }
+ string_list_clear(&needs_pushing, 0);
+ sha1_array_clear(&commits);
}
push_ret = transport->push_refs(transport, remote_refs, flags);
--
2.10.1.386.gc503e45
^ permalink raw reply related
* [PATCH v4 4/4] submodule_needs_pushing() NEEDSWORK when we can not answer this question
From: Heiko Voigt @ 2016-11-16 15:11 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, git, Jeff King, Stefan Beller, Jens.Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <cover.1479308877.git.hvoigt@hvoigt.net>
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
submodule.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/submodule.c b/submodule.c
index 11391fa..00dd655 100644
--- a/submodule.c
+++ b/submodule.c
@@ -531,6 +531,17 @@ static int submodule_has_commits(const char *path, struct sha1_array *commits)
static int submodule_needs_pushing(const char *path, struct sha1_array *commits)
{
if (!submodule_has_commits(path, commits))
+ /*
+ * NOTE: We do consider it safe to return "no" here. The
+ * correct answer would be "We do not know" instead of
+ * "No push needed", but it is quite hard to change
+ * the submodule pointer without having the submodule
+ * around. If a user did however change the submodules
+ * without having the submodule around, this indicates
+ * an expert who knows what they are doing or a
+ * maintainer integrating work from other people. In
+ * both cases it should be safe to skip this check.
+ */
return 0;
if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
--
2.10.1.386.gc503e45
^ permalink raw reply related
* [PATCH v4 1/4] serialize collection of changed submodules
From: Heiko Voigt @ 2016-11-16 15:11 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, git, Jeff King, Stefan Beller, Jens.Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <cover.1479308877.git.hvoigt@hvoigt.net>
To check whether a submodule needs to be pushed we need to collect all
changed submodules. Lets collect them first and then execute the
possibly expensive test whether certain revisions are already pushed
only once per submodule.
There is further potential for optimization since we can assemble one
command and only issued that instead of one call for each remote ref in
the submodule.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
submodule.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 55 insertions(+), 4 deletions(-)
diff --git a/submodule.c b/submodule.c
index 6f7d883..b2908fe 100644
--- a/submodule.c
+++ b/submodule.c
@@ -532,19 +532,34 @@ static int submodule_needs_pushing(const char *path, const unsigned char sha1[20
return 0;
}
+static struct sha1_array *submodule_commits(struct string_list *submodules,
+ const char *path)
+{
+ struct string_list_item *item;
+
+ item = string_list_insert(submodules, path);
+ if (item->util)
+ return (struct sha1_array *) item->util;
+
+ /* NEEDSWORK: should we have sha1_array_init()? */
+ item->util = xcalloc(1, sizeof(struct sha1_array));
+ return (struct sha1_array *) item->util;
+}
+
static void collect_submodules_from_diff(struct diff_queue_struct *q,
struct diff_options *options,
void *data)
{
int i;
- struct string_list *needs_pushing = data;
+ struct string_list *submodules = data;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
+ struct sha1_array *commits;
if (!S_ISGITLINK(p->two->mode))
continue;
- if (submodule_needs_pushing(p->two->path, p->two->oid.hash))
- string_list_insert(needs_pushing, p->two->path);
+ commits = submodule_commits(submodules, p->two->path);
+ sha1_array_append(commits, p->two->oid.hash);
}
}
@@ -560,6 +575,30 @@ static void find_unpushed_submodule_commits(struct commit *commit,
diff_tree_combined_merge(commit, 1, &rev);
}
+struct collect_submodule_from_sha1s_data {
+ char *submodule_path;
+ struct string_list *needs_pushing;
+};
+
+static int collect_submodules_from_sha1s(const unsigned char sha1[20],
+ void *data)
+{
+ struct collect_submodule_from_sha1s_data *me = data;
+
+ if (submodule_needs_pushing(me->submodule_path, sha1))
+ string_list_insert(me->needs_pushing, me->submodule_path);
+
+ return 0;
+}
+
+static void free_submodules_sha1s(struct string_list *submodules)
+{
+ struct string_list_item *item;
+ for_each_string_list_item(item, submodules)
+ sha1_array_clear((struct sha1_array *) item->util);
+ string_list_clear(submodules, 1);
+}
+
int find_unpushed_submodules(unsigned char new_sha1[20],
const char *remotes_name, struct string_list *needs_pushing)
{
@@ -568,6 +607,8 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
int argc = ARRAY_SIZE(argv) - 1;
char *sha1_copy;
+ struct string_list submodules = STRING_LIST_INIT_DUP;
+ struct string_list_item *submodule;
struct strbuf remotes_arg = STRBUF_INIT;
@@ -581,12 +622,22 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
die("revision walk setup failed");
while ((commit = get_revision(&rev)) != NULL)
- find_unpushed_submodule_commits(commit, needs_pushing);
+ find_unpushed_submodule_commits(commit, &submodules);
reset_revision_walk();
free(sha1_copy);
strbuf_release(&remotes_arg);
+ for_each_string_list_item(submodule, &submodules) {
+ struct collect_submodule_from_sha1s_data data;
+ data.submodule_path = submodule->string;
+ data.needs_pushing = needs_pushing;
+ sha1_array_for_each_unique((struct sha1_array *) submodule->util,
+ collect_submodules_from_sha1s,
+ &data);
+ }
+ free_submodules_sha1s(&submodules);
+
return needs_pushing->nr;
}
--
2.10.1.386.gc503e45
^ permalink raw reply related
* [PATCH v4 0/4] Speedup finding of unpushed submodules
From: Heiko Voigt @ 2016-11-16 15:11 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, git, Jeff King, Stefan Beller, Jens.Lehmann,
Fredrik Gustafsson, Leandro Lucarella
You can find the third iteration of this series here:
http://public-inbox.org/git/cover.1479221071.git.hvoigt@hvoigt.net/
All comments from the last iteration should be addressed.
Cheers Heiko
Heiko Voigt (4):
serialize collection of changed submodules
serialize collection of refs that contain submodule changes
batch check whether submodule needs pushing into one call
submodule_needs_pushing() NEEDSWORK when we can not answer this
question
submodule.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++-------------
submodule.h | 5 ++-
transport.c | 29 ++++++++++----
3 files changed, 121 insertions(+), 36 deletions(-)
--
2.10.1.386.gc503e45
^ permalink raw reply
* Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS
From: Heiko Voigt @ 2016-11-16 14:39 UTC (permalink / raw)
To: Jeff King
Cc: Lars Schneider, Junio C Hamano, Torsten Bögershausen, git,
Eric Sunshine
In-Reply-To: <20161115153159.mxfl73dnhljad5so@sigill.intra.peff.net>
On Tue, Nov 15, 2016 at 10:31:59AM -0500, Jeff King wrote:
> On Tue, Nov 15, 2016 at 01:07:18PM +0100, Heiko Voigt wrote:
>
> > On Fri, Nov 11, 2016 at 09:22:51AM +0100, Lars Schneider wrote:
> > > To all macOS users on the list:
> > > Does anyone execute the tests with GIT_TEST_HTTPD enabled successfully?
> >
> > Nope. The following tests fail for me on master: 5539, 5540, 5541, 5542,
> > 5550, 5551, 5561, 5812.
>
> Failing how? Does apache fail to start up? Do tests fails? What does
> "-v" say? Is there anything interesting in httpd/error.log in the trash
> directory?
This is what I see for 5539:
$ GIT_TEST_HTTPD=1 ./t5539-fetch-http-shallow.sh -v
Initialized empty Git repository in /Users/hvoigt/Repository/git4/t/trash directory.t5539-fetch-http-shallow/.git/
checking prerequisite: NOT_ROOT
mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" &&
(
cd "$TRASH_DIRECTORY/prereq-test-dir" &&
uid=$(id -u) &&
test "$uid" != 0
)
prerequisite NOT_ROOT ok
httpd: Syntax error on line 65 of /Users/hvoigt/Repository/git4/t/lib-httpd/apache.conf: Cannot load modules/mod_mpm_prefork.so into server: dlopen(/Users/hvoigt/Repository/git4/t/trash directory.t5539-fetch-http-shallow/httpd/modules/mod_mpm_prefork.so, 10): image not found
error: web server setup failed
It seems the other failures have the same cause.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH v3 3/4] batch check whether submodule needs pushing into one call
From: Heiko Voigt @ 2016-11-16 14:29 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, git@vger.kernel.org, Jeff King, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <CAGZ79kZtQG5pE-6N-1yZKA95VRmrDRB3PnSd1gtqBM9fxD48Cg@mail.gmail.com>
On Tue, Nov 15, 2016 at 02:28:31PM -0800, Stefan Beller wrote:
> On Tue, Nov 15, 2016 at 6:56 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
>
> > -static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])
> > +static int check_has_commit(const unsigned char sha1[20], void *data)
> > {
> > - if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
> > + int *has_commit = (int *) data;
>
> nit: just as prior patches ;) void* can be cast implicitly.
Even though its just a nit: Will remove all the void casts. :)
Cheers Heiko
^ permalink raw reply
* Re: [PATCH v3 4/4] submodule_needs_pushing() NEEDSWORK when we can not answer this question
From: Heiko Voigt @ 2016-11-16 14:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, git@vger.kernel.org, Jeff King, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <xmqqtwb8s2m8.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 15, 2016 at 04:13:51PM -0800, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
> >> "We do not know" ...
> >
> > ... because there is no way to check for us as we don't have the
> > submodule commits.
> >
> > " We do consider it safe as no one in their sane mind would
> > have changed the submodule pointers without having the
> > submodule around. If a user did however change the submodules
> > without having the submodule commits around, this indicates an
> > expert who knows what they were doing."
>
> I didn't think it through myself to arrive at such a conclusion, but
> to me the above sounds like a sensible reasoning [*1*].
I think you have a point here. If I rephrase it like this: "We do
consider it safe as no one in their sane mind *could* have changed the
submodule pointers without having the submodule around..."
Since its actually hard to create such a situation without the submodule
commit around I agree here.
> *1* My version was more like "we do not know if they would get into
> a situation where they do not have enough submodule commits if
> we pushed our superproject, but more importantly, we DO KNOW
> that it would not help an iota if we pushed our submodule to
> them, so there is no point stopping the push of superproject
> saying 'no, no, no, you must push the submodule first'".
Yes saying that would be wrong. I was rather suggesting that we tell the
user that we could not find the submodule commits to and that if he
wants to proceed he should either pass --recurse-submodules=no or
initialize the submodule.
But I think the above reasoning obsoletes my suggestion. I would adjust
the comment accordingly but still keep the patch so we have
documentation that this behavior is on purpose.
Cheers Heiko
^ permalink raw reply
* Re: [RFH] limiting ref advertisements
From: Duy Nguyen @ 2016-11-16 13:42 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161114212122.rkgeoh4rj5cxdadf@sigill.intra.peff.net>
On Tue, Nov 15, 2016 at 4:21 AM, Jeff King <peff@peff.net> wrote:
> Thanks for responding to this.
Glad to help (or more precisely annoy you somewhat :D)
> I've been meaning to get back to it with
> some code experiments, but they keep getting bumped down in priority. So
> let me at least outline some of my thoughts, without code. :)
>
> I was hoping to avoid right-anchoring because it's expensive to find all
> of the right-anchored cases (assuming that ref storage is generally
> hierarchical, which it is now and probably will be for future backends).
Urgh.. I completely forgot about future refs backends. Yeah this would
kick both wildmatch and right-anchoring out of the window.
For the record I almost suggested BPF as well (to keep the server side
simple, but at the super high cost of client side). That would also go
out of the window the same way wildmatch and right-anchoring does.
> But remember that these are "early capabilities", before the server
> has spoken at all. So the client doesn't know if we can handle v2.
> So we have to send _both_ (and v2-aware servers can ignore the v1).
>
> advertise-lookup-v1=master
> advertise-lookup-v2=master
>
> But that's not quite enough. A v1 server won't look in refs/notes
> at all. So we have to say that, too:
>
> advertise-lookup-v1=refs/notes/master
>
> And of course the v1 server has no idea that this isn't necessary
> if we already found refs/heads/master.
We discussed a bit about upgrading upload-pack version 1 to 2 in more
than one session: the first fetch just does v1 as normal, the server
returns v1 response to but also advertises that v2 is supported. The
client keeps this info and skips v1 and tries v2 right away in the
following fetches, falling back to v1 (new fetch session) if v2 is
unsupported. Can it work the same way here too?
I'm in favor of this option 2 (without trying to be absolute backward
compatible with older lookup versions) since it allows us to optimize
for common case and experiment a bit. Once we know better we can make
the next version that hopefully suits everybody.
> So I think you really do need the client to be able to say "also
> look at this pattern".
What about the order of patterns? Does it matter "this pattern" is in
the middle or the end of the pattern list? I suppose not, just
checking...
But does this call for the ability to remove a pattern from the
pattern list as well, as a way to narrow down the search scope and
avoid sending unwanted refs?
> Of course we do still want left-anchoring, too. Wildcards like
> "refs/heads/*" are always left-anchored. So I think we'd have two types,
> and a full request for
>
> git fetch origin +refs/heads/*:refs/remotes/origin/* master:foo
>
> would look like:
>
> (1) advertise-pattern-v1
> (2) advertise-pattern=refs/notes/%s
> (3) advertise-prefix=refs/heads
> (4) advertise-lookup=master
>
> where the lines mean:
>
> 1. Use the standard v1 patterns (we could spell them out, but this
> just saves bandwidth. In fact, it could just be implicit that v1
> patterns are included, and we could skip this line).
>
> 2. This is for our fictional future version where the client knows
> added refs/notes/* to its DWIM but the server hasn't yet.
>
> 3. Give me all of refs/heads/*
>
> 4. Look up "master" using the advertise patterns and give me the first
> one you find.
Well.. it sounds good to me. But I would not trust myself on refs matters :D
> So given that we can omit (1), and that (2) is just an example for the
> future, it could look like:
>
> advertise-prefix=refs/heads
> advertise-lookup=master
>
> which is pretty reasonable. It's not _completely_ bulletproof in terms
> of backwards compatibility. The "v1" thing means the client can't insert
> a new pattern in the middle (remember they're ordered by priority).
OK so pattern order probably matters...
> So
> maybe it is better to spell them all out (one thing that makes me
> hesitate is that these will probably end up as URL parameters for the
> HTTP version, which means our URL can start to get a little long).
>
> Anyway. That's the direction I'm thinking. I haven't written the code
> yet. The trickiest thing will probably be that the server would want to
> avoid advertising the same ref twice via two mechanisms (or perhaps the
> client just be tolerant of duplicates; that relieves the server of any
> duplicate-storage requirements).
Thanks for sharing.
--
Duy
^ permalink raw reply
* Re: [PATCH] worktree: fix a sparse 'Using plain integer as NULL pointer' warning
From: Duy Nguyen @ 2016-11-16 13:17 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <5b7d7d0b-8a6c-d516-4eb9-4e4ea13dce73@ramsayjones.plus.com>
On Wed, Nov 16, 2016 at 3:28 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>
> Hi Duy,
>
> If you need to re-roll your 'nd/worktree-move' branch, could you
> please squash this into the relevant patch [commit c49e92f5c
> ("worktree move: refuse to move worktrees with submodules", 12-11-2016)].
Will do, thanks (and it's also "thanks" for your other similar emails,
I just don't want to send a mail with just 'thanks' that adds nothing
else).
> Also, one of the new tests introduced by commit 31a8f3066 ("worktree move:
> new command", 12-11-2016), fails for me, thus:
>
> $ ./t2028-worktree-move.sh -i -v
> ...
> --- expected 2016-11-15 20:22:50.647241458 +0000
> +++ actual 2016-11-15 20:22:50.647241458 +0000
> @@ -1,3 +1,3 @@
> worktree /home/ramsay/git/t/trash directory.t2028-worktree-move
> -worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/destination
> worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/elsewhere
> +worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/destination
> not ok 12 - move worktree
> #
> # git worktree move source destination &&
> # test_path_is_missing source &&
> # git worktree list --porcelain | grep "^worktree" >actual &&
> # cat <<-EOF >expected &&
> # worktree $TRASH_DIRECTORY
> # worktree $TRASH_DIRECTORY/destination
> # worktree $TRASH_DIRECTORY/elsewhere
> # EOF
> # test_cmp expected actual &&
> # git -C destination log --format=%s >actual2 &&
> # echo init >expected2 &&
> # test_cmp expected2 actual2
> #
> $
>
> Is there an expectation that the submodules will be listed in
> any particular order by 'git worktree list --porcelain' ?
I just sent a patch [1] to fix this before reading this mail. The
order so far has been determined by readdir() which is not great.
[1] https://public-inbox.org/git/CACsJy8DOT_4N_48UaoYK61G_8JUaXbEs7N=n24CH2q1GN=++5g@mail.gmail.com/T/#mfcf797219a1a143ed2ac45198015f19e82c70db2
--
Duy
^ permalink raw reply
* Re: Git status takes too long- How to improve the performance of git
From: Fredrik Gustafsson @ 2016-11-16 13:21 UTC (permalink / raw)
To: Renuka Pampana; +Cc: Fredrik Gustafsson, git
In-Reply-To: <CAEAva_1JAu+kWmk3MZDFK=4CgQB5M+JN8FwzMVr6zKgXTAhdXw@mail.gmail.com>
On Wed, Nov 16, 2016 at 05:13:57PM +0530, Renuka Pampana wrote:
> > On Tue, Nov 15, 2016 at 02:33:12AM -0700, ravalika wrote:
> > > It is an centralized server and git status takes too long
> >
> > A centralized server? How? git is designed to be runned locally. If
> > you're running git on a network file system, the performance will
> > suffer. Could you elaborate on how your environment is setup?
> >
> >
> We have setup main git repository in remote location on Linux server
> And created a git repository in local Linux server, as a reference for the
> remote git repository,
> And update the local git repository for every 15 min in local server
>
> Users will be able to access the local git repository through NFS
And each user will have their own copy of the repository locally on
their machine? That is having done a git clone?
>
> All users will clone the git repository from remote project url by using
> local git repo as reference
>
> For example : git clone --reference ~/gitcaches/reference user@drupal
> :/home/project/drupal.git
>
> All the users have ssh credentials for the remote server
Why are you using --reference for a 8.9MB big clone?
>
>
> What is the best way to implement remote git repo and able to access the
> git repo from other location, without any performance glitches?
> Users should be able to access git repo from different servers and from
> different locations.
The best way is to have it locally cloned. Yes the initial clone will be
expensive but operations after that will be fairly smooth. You do not(!)
want to execute git on one machine and having the repository beeing on
an other machine (for example via a network file system, except git
clone, git fetch, git push, etc.).
> >
> > > How to improve the performance of git status
> > >
> > > Git repo details:
> > >
> > > Size of the .git folder is 8.9MB
> > > Number of commits approx 53838 (git rev-list HEAD --count)
> > > Number of branches - 330
> > > Number of files - 63883
> > > Working tree clone size is 4.3GB
> >
> > .git folder of 8.9 MEGABYTE and working tree of 4.3 GIGABYTE? Is this a
> > typo?
> >
> > All git related information is stored in .git directory of the working
> directory
> It is 8.9M
> And size of the local workspace is 4.3G
Can you please elaborate on this? How can you store 8.9 MB of data that
will result in a 4.3 G workspace?
--
Fredrik Gustafsson
phone: +46 733-608274
e-mail: iveqy@iveqy.com
website: http://www.iveqy.com
^ permalink raw reply
* Re: [PATCH 00/11] git worktree (re)move
From: Duy Nguyen @ 2016-11-16 13:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <20161116130533.GA5393@ash>
On Wed, Nov 16, 2016 at 8:05 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> diff --git a/worktree.c b/worktree.c
> index f7869f8..fe92d6f 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -173,6 +173,13 @@ static void mark_current_worktree(struct worktree **worktrees)
> free(git_dir);
> }
>
> +static int compare_worktree(const void *a_, const void *b_)
> +{
> + const struct worktree *const *a = a_;
> + const struct worktree *const *b = b_;
> + return fspathcmp((*a)->path, (*b)->path);
> +}
> +
> struct worktree **get_worktrees(void)
> {
> struct worktree **list = NULL;
> @@ -205,6 +212,11 @@ struct worktree **get_worktrees(void)
> ALLOC_GROW(list, counter + 1, alloc);
> list[counter] = NULL;
>
> + /*
> + * don't sort the first item (main worktree), which will
> + * always be the first
> + */
Urgh.. I should review my patches more carefully before sending out :(
The main worktree could be missing (failing to parse HEAD) so I need a
better trick than simply assuming the first item is the main worktree
here. Tests did not catch this, naturally..
> + qsort(list + 1, counter - 1, sizeof(*list), compare_worktree);
> mark_current_worktree(list);
> return list;
> }
--
Duy
^ permalink raw reply
* Re: [PATCH 00/11] git worktree (re)move
From: Duy Nguyen @ 2016-11-16 13:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqa8d4yts7.fsf@gitster.mtv.corp.google.com>
On Sat, Nov 12, 2016 at 06:53:44PM -0800, Junio C Hamano wrote:
> not ok 12 - move worktree
> #
> # git worktree move source destination &&
> # test_path_is_missing source &&
> # git worktree list --porcelain | grep "^worktree" >actual &&
> # cat <<-EOF >expected &&
> # worktree $TRASH_DIRECTORY
> # worktree $TRASH_DIRECTORY/destination
> # worktree $TRASH_DIRECTORY/elsewhere
> # EOF
> # test_cmp expected actual &&
> # git -C destination log --format=%s >actual2 &&
> # echo init >expected2 &&
> # test_cmp expected2 actual2
I think I've seen this (i.e. 'expected' and 'actual' differ only in
the order of items) once after a rebase and ignored it, assuming
something was changed during the rebase that caused this.
The following patch should fix it if that's the same thing you saw. I
could pile it on worktree-move series, or you can make it a separate
one-patch series. What's your preference?
-- 8< --
Subject: [PATCH] worktree list: keep the list sorted
It makes it easier to write tests for. But it should also be good for
the user since locating a worktree by eye would be easier once they
notice this.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
worktree.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/worktree.c b/worktree.c
index f7869f8..fe92d6f 100644
--- a/worktree.c
+++ b/worktree.c
@@ -173,6 +173,13 @@ static void mark_current_worktree(struct worktree **worktrees)
free(git_dir);
}
+static int compare_worktree(const void *a_, const void *b_)
+{
+ const struct worktree *const *a = a_;
+ const struct worktree *const *b = b_;
+ return fspathcmp((*a)->path, (*b)->path);
+}
+
struct worktree **get_worktrees(void)
{
struct worktree **list = NULL;
@@ -205,6 +212,11 @@ struct worktree **get_worktrees(void)
ALLOC_GROW(list, counter + 1, alloc);
list[counter] = NULL;
+ /*
+ * don't sort the first item (main worktree), which will
+ * always be the first
+ */
+ qsort(list + 1, counter - 1, sizeof(*list), compare_worktree);
mark_current_worktree(list);
return list;
}
--
2.8.2.524.g6ff3d78
-- 8< --
^ permalink raw reply related
* Re: RFC: Enable delayed responses to Git clean/smudge filter requests
From: Lars Schneider @ 2016-11-16 9:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Wong, git
In-Reply-To: <xmqqvavotych.fsf@gitster.mtv.corp.google.com>
On 15 Nov 2016, at 19:03, Junio C Hamano <gitster@pobox.com> wrote:
> Lars Schneider <larsxschneider@gmail.com> writes:
>
>>> The filter itself would need to be aware of parallelism
>>> if it lives for multiple objects, right?
>>
>> Correct. This way Git doesn't need to deal with threading...
>
> I think you need to be careful about three things (at least; there
> may be more):
>
> * Codepaths that check out multiple cache entries do rely on the
> order of checkout. We checkout removals first to make room so
> that creation of a path X can succeed if an existing path X/Y
> that used to want to see X as a directory can succeed (see the
> use of checkout_entry() by "git checkout", which does have two
> separate loops to explicitly guarantee this), for example. I
> think "remove all and then create" you do not specifically have
> to worry about with the proposed change, but you may need to
> inspect and verify there aren't other kind of order dependency.
OK
> * Done naively, it will lead to unmaintainable code, like this:
>
> + struct list_of_cache_entries *list = ...;
> for (i = 0; i < active_nr; i++)
> - checkout_entry(active_cache[i], state, NULL);
> + if (checkout_entry(active_cache[i], state, NULL) == DELAYED)
> + add_cache_to_queue(&list, active_cache[i]);
> + while (list) {
> + wait_for_checkout_to_finish(*list);
> + list = list->next;
> + }
>
> I do not think we want to see such a rewrite all over the
> codepaths. It might be OK to add such a "these entries are known
> to be delayed" list in struct checkout so that the above becomes
> more like this:
>
> for (i = 0; i < active_nr; i++)
> checkout_entry(active_cache[i], state, NULL);
> + checkout_entry_finish(state);
>
> That is, addition of a single "some of the checkout_entry() calls
> done so far might have been lazy, and I'll give them a chance to
> clean up" might be palatable. Anything more than that on the
> caller side is not.
I haven't thought hard about the implementation, yet, but I'll try
to stick to your suggestion and change as less code as possible on
the caller sides.
> * You'd need to rein in the maximum parallelism somehow, as you do
> not want to see hundreds of competing filter processes starting
> only to tell the main loop over an index with hundreds of entries
> that they are delayed checkouts.
I intend to implement this feature only for the new long running filter
process protocol. OK with you?
Thanks,
Lars
^ permalink raw reply
* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Johannes Schindelin @ 2016-11-16 9:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael J Gruber, Jacob Keller, Dennis Kaarsemaker,
Git mailing list
In-Reply-To: <xmqqtwb9wywp.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 14 Nov 2016, Junio C Hamano wrote:
> I _think_ the no-index mode was primarily for those who want to use
> our diff as a replacement for GNU and other diffs, and from that
> point of view, I'd favour not doing the "comparing symbolic link?
> We'll show the difference between the link contents, not target"
> under no-index mode myself.
If I read this correctly, then we are in agreement that the default for
--no-index should be as it is right now, i.e. comparing symlink targets as
opposed to --follow-links.
> That is a lot closer to the diff other people implemented, not ours.
> Hence the knee-jerk reaction I gave in
>
> http://public-inbox.org/git/xmqqinrt1zcx.fsf@gitster.mtv.corp.google.com
Let me quote the knee-jerk reaction:
> My knee-jerk reaction is:
>
> * The --no-index mode should default to your --follow-symlinks
> behaviour, without any option to turn it on or off.
But this is the exact opposite of what I find reasonable.
Ciao,
Johannes
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox