* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Junio C Hamano @ 2016-11-16 18:37 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Michael J Gruber, Jacob Keller, Dennis Kaarsemaker,
Git mailing list
In-Reply-To: <xmqqh977qnvk.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> 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,...
>
> Now I re-read it and I can see it can be read either way.
>
> By "link contents" in "comparing symbolic link? We'll show the
> difference between the link contents, not target", I meant the
> result you get from readlink(2), which will result in
>
> diff --git a/RelNotes b/RelNotes
> index c02235fe8c..b54330f7cd 120000
> --- a/RelNotes
> +++ b/RelNotes
> @@ -1 +1 @@
> -Documentation/RelNotes/2.10.2.txt
> \ No newline at end of file
> +Documentation/RelNotes/2.11.0.txt
> \ No newline at end of file
>
> not the comparison between the files that are link targets,
> i.e. hypothetical
>
> diff --git a/RelNotes b/RelNotes
> index c4d4397023..7a1fce7720 100644
> --- a/Documentation/RelNotes/2.10.2.txt
> +++ b/Documentation/RelNotes/2.11.0.txt
> @@ -1,41 +1,402 @@
> -Git v2.10.2 Release Notes
> -=========================
> +Git 2.11 Release Notes
> ...
>
> And I'd favour *NOT* doing that if we are using our diff as a
Again, this can be read both ways. By "that" on the above line I
meant "the former".
> replacement for GNU and other diffs in "no-index" mode. Which leads
> to ...
>
>>> 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
>
> ... this conclusion, which is consistent with ...
>
>>
>> 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.
>
> ... this one.
>
> But notice "I _think_" in the first sentence you quoted. That is a
> basic assumption that leads to the conclusion, and that assumption
> is not a fact. Maybe users do *not* want the "no-index" mode as a
> replacement for GNU and other diffs, in which case comparing the
> result of readlink(2) even in no-index mode might have merit. I
> just didn't think it was the case.
And "I just didn't think it was the case", when fully spelt out, is
"I just didn't think that the assumption was incorrect."
^ permalink raw reply
* [ANNOUNCE] Git Rev News edition 20
From: Christian Couder @ 2016-11-16 18:35 UTC (permalink / raw)
To: git
Cc: lwn, Junio C Hamano, Johannes Schindelin,
Thomas Ferris Nicolaisen, Jakub Narebski, Jeff King,
Stefan Beller, Jacob Keller, Aaron Pelly, Nguyen Thai Ngoc Duy,
Alexei Lozovsky, Martin Braun, Brendan Forster, Parker Moore,
Shawn Pearce, Jonathan Nieder, Jonathan Tan, Brandon Williams,
Markus Jansen, SZEDER Gábor
Hi everyone,
I'm happy announce that the 21th edition of Git Rev News is now published:
https://git.github.io/rev_news/2016/11/16/edition-21/
Thanks a lot to all the contributors and helpers, especially Jacob,
Dscho, Markus, Gábor and Peff!
Enjoy,
Christian, Thomas and Jakub.
^ permalink raw reply
* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Junio C Hamano @ 2016-11-16 18:29 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Michael J Gruber, Jacob Keller, Dennis Kaarsemaker,
Git mailing list
In-Reply-To: <alpine.DEB.2.20.1611161039590.3746@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 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,...
Now I re-read it and I can see it can be read either way.
By "link contents" in "comparing symbolic link? We'll show the
difference between the link contents, not target", I meant the
result you get from readlink(2), which will result in
diff --git a/RelNotes b/RelNotes
index c02235fe8c..b54330f7cd 120000
--- a/RelNotes
+++ b/RelNotes
@@ -1 +1 @@
-Documentation/RelNotes/2.10.2.txt
\ No newline at end of file
+Documentation/RelNotes/2.11.0.txt
\ No newline at end of file
not the comparison between the files that are link targets,
i.e. hypothetical
diff --git a/RelNotes b/RelNotes
index c4d4397023..7a1fce7720 100644
--- a/Documentation/RelNotes/2.10.2.txt
+++ b/Documentation/RelNotes/2.11.0.txt
@@ -1,41 +1,402 @@
-Git v2.10.2 Release Notes
-=========================
+Git 2.11 Release Notes
...
And I'd favour *NOT* doing that if we are using our diff as a
replacement for GNU and other diffs in "no-index" mode. Which leads
to ...
>> 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
... this conclusion, which is consistent with ...
>
> 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.
... this one.
But notice "I _think_" in the first sentence you quoted. That is a
basic assumption that leads to the conclusion, and that assumption
is not a fact. Maybe users do *not* want the "no-index" mode as a
replacement for GNU and other diffs, in which case comparing the
result of readlink(2) even in no-index mode might have merit. I
just didn't think it was the case.
^ permalink raw reply
* Re: Protecting old temporary objects being reused from concurrent "git gc"?
From: Junio C Hamano @ 2016-11-16 18:18 UTC (permalink / raw)
To: Jeff King; +Cc: Matt McCutchen, git
In-Reply-To: <20161116080753.gkn6v7vhdbifpubn@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... 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.
Yeah, I missed that. It says "we were asked to update one
cache_tree that corresponds to this subdirectory, found that
hashes everything below has been rolled up and still valid, and we
already have the right tree object in the object store".
It can simply become freshen(), which is "do we have it in the
object store?" with a side effect of touching iff the answer is
"yes".
^ permalink raw reply
* Re: RFC: Enable delayed responses to Git clean/smudge filter requests
From: Junio C Hamano @ 2016-11-16 18:15 UTC (permalink / raw)
To: Lars Schneider; +Cc: Eric Wong, git
In-Reply-To: <17709AFF-3C2D-4EC0-97DC-BD750F514D0B@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
>> * 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?
Do you mean that a long-running filter process interacting with
convert_to_worktree() called from checkout_entry() will be the only
codepath that will spawn multiple processes or threads?
That is fine, but it does not change the fact that you still need to
limit the maximum parallelism there.
^ permalink raw reply
* Re: [PATCH 00/11] git worktree (re)move
From: Junio C Hamano @ 2016-11-16 18:11 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <20161116130533.GA5393@ash>
Duy Nguyen <pclouds@gmail.com> writes:
> 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?
Giving a stable output to the users is probably a good preparatory
fix to what is already in the released versions, so it would make
the most sense to make it a separate patch to be applied to maint
then build the remainder on top.
I do not think "always show the primary first" is necessarily a good
idea (I would have expected an output more like "git branch --list").
^ permalink raw reply
* [PATCH] rev-parse: fix parent shorthands with --symbolic
From: Jeff King @ 2016-11-16 8:46 UTC (permalink / raw)
To: git
The try_parent_shorthands() function shows each parent via
show_rev(). We pass the correct parent sha1, but our "name"
parameter still points at the original refname. So asking
for a regular rev-parse works fine (it prints the sha1s),
but asking for the symbolic name gives nonsense like:
$ git rev-parse --symbolic HEAD^-1
HEAD
^HEAD
which is always an empty set of commits. Asking for "^!" is
likewise broken, with the added bonus that its prints ^HEAD
for _each_ parent. And "^@" just prints HEAD repeatedly.
Arguably it would be correct to just pass NULL as the name
here, and always get the parent expressed as a sha1. The
"--symbolic" documentaton claims only "as close to the
original input as possible", and we certainly fallback to
sha1s where necessary. But it's pretty easy to generate a
symbolic name on the fly from the original.
Signed-off-by: Jeff King <peff@peff.net>
---
I noticed this because tig uses "--symbolic", and "tig 1234abcd^-1" does
not work without this patch.
I thought at first it might be a regression in the upcoming v2.11 from
8779351dd (revision: new rev^-n shorthand for rev^n..rev, 2016-09-27).
But nope, "^!" has been broken at least as far back as v1.6.6.3 (I
didn't check further).
So assuming it's too late in the -rc cycle for a non-regression fix,
this is probably post-v2.11 maint material.
builtin/rev-parse.c | 7 ++++++-
t/t6101-rev-parse-parents.sh | 18 ++++++++++++++++++
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index cfb0f1510..ff13e59e1 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -342,11 +342,16 @@ static int try_parent_shorthands(const char *arg)
for (parents = commit->parents, parent_number = 1;
parents;
parents = parents->next, parent_number++) {
+ char *name = NULL;
+
if (exclude_parent && parent_number != exclude_parent)
continue;
+ if (symbolic)
+ name = xstrfmt("%s^%d", arg, parent_number);
show_rev(include_parents ? NORMAL : REVERSED,
- parents->item->object.oid.hash, arg);
+ parents->item->object.oid.hash, name);
+ free(name);
}
*dotdot = '^';
diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
index 64a9850e3..8c617981a 100755
--- a/t/t6101-rev-parse-parents.sh
+++ b/t/t6101-rev-parse-parents.sh
@@ -83,12 +83,24 @@ test_expect_success 'final^1^@ = final^1^1 final^1^2' '
test_cmp expect actual
'
+test_expect_success 'symbolic final^1^@ = final^1^1 final^1^2' '
+ git rev-parse --symbolic final^1^1 final^1^2 >expect &&
+ git rev-parse --symbolic final^1^@ >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'final^1^! = final^1 ^final^1^1 ^final^1^2' '
git rev-parse final^1 ^final^1^1 ^final^1^2 >expect &&
git rev-parse final^1^! >actual &&
test_cmp expect actual
'
+test_expect_success 'symbolic final^1^! = final^1 ^final^1^1 ^final^1^2' '
+ git rev-parse --symbolic final^1 ^final^1^1 ^final^1^2 >expect &&
+ git rev-parse --symbolic final^1^! >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'large graft octopus' '
test_cmp_rev_output b31 "git rev-parse --verify b1^30"
'
@@ -143,6 +155,12 @@ test_expect_success 'rev-parse merge^-2 = merge^2..merge' '
test_cmp expect actual
'
+test_expect_success 'symbolic merge^-1 = merge^1..merge' '
+ git rev-parse --symbolic merge^1..merge >expect &&
+ git rev-parse --symbolic merge^-1 >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'rev-parse merge^-0 (invalid parent)' '
test_must_fail git rev-parse merge^-0
'
--
2.11.0.rc1.280.gf9bb6f9
^ permalink raw reply related
* 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
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