From: Derrick Stolee <stolee@gmail.com>
To: Stefan Beller <sbeller@google.com>, git@vger.kernel.org
Cc: szeder.dev@gmail.com, jonathantanmy@google.com
Subject: Re: [PATCH 19/24] submodule: use submodule repos for object lookup
Date: Fri, 2 Nov 2018 09:03:02 -0400 [thread overview]
Message-ID: <127add9b-d58c-ee46-73de-41bfe915589b@gmail.com> (raw)
In-Reply-To: <20181030220817.61691-20-sbeller@google.com>
On 10/30/2018 6:08 PM, Stefan Beller wrote:
> This converts the 'show_submodule_header' function to use
> the repository API properly, such that the submodule objects
> are not added to the main object store.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
A couple tests are broken in 'pu' when run with GIT_TEST_COMMIT_GRAPH=1,
including t4041-diff-submodule-option.sh. The failure bisects to this patch.
Here is a verbose output of the first failure in that script:;
expecting success:
git diff-index -p --submodule=log HEAD >actual &&
cat >expected <<-EOF &&
Submodule sm1 $head2..$head3 (rewind):
< Add foo3 ($added foo3)
< Add foo2 ($added foo2)
EOF
test_cmp expected actual
+ git diff-index -p --submodule=log HEAD
+ cat
+ test_cmp expected actual
+ diff -u expected actual
--- expected 2018-11-02 12:58:43.429262380 +0000
+++ actual 2018-11-02 12:58:43.429262380 +0000
@@ -1,3 +1,5 @@
-Submodule sm1 30b9670..dafb207 (rewind):
+Submodule sm1 30b9670...dafb207:
< Add foo3 (hinzugefügt foo3)
< Add foo2 (hinzugefügt foo2)
+ > Add foo1 (hinzugefügt foo1)
+ < Add foo1 (hinzugefügt foo1)
error: last command exited with $?=1
not ok 9 - modified submodule(backward)
I've been looking into the patch below to see if there is an obvious
problem, but the best I can think is that open_submodule() creates an
alternate 'struct repository' and somehow the commit-graph feature is
interacting poorly with that struct.
Stefan, do you know what's going on?
Thanks,
-Stolee
> ---
> submodule.c | 76 ++++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 61 insertions(+), 15 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index d9d3046689..0fdda45252 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -443,7 +443,7 @@ static int prepare_submodule_summary(struct rev_info *rev, const char *path,
> return prepare_revision_walk(rev);
> }
>
> -static void print_submodule_summary(struct rev_info *rev, struct diff_options *o)
> +static void print_submodule_summary(struct repository *r, struct rev_info *rev, struct diff_options *o)
> {
> static const char format[] = " %m %s";
> struct strbuf sb = STRBUF_INIT;
> @@ -454,7 +454,8 @@ static void print_submodule_summary(struct rev_info *rev, struct diff_options *o
> ctx.date_mode = rev->date_mode;
> ctx.output_encoding = get_log_output_encoding();
> strbuf_setlen(&sb, 0);
> - format_commit_message(commit, format, &sb, &ctx);
> + repo_format_commit_message(r, commit, format, &sb,
> + &ctx);
> strbuf_addch(&sb, '\n');
> if (commit->object.flags & SYMMETRIC_LEFT)
> diff_emit_submodule_del(o, sb.buf);
> @@ -481,14 +482,47 @@ void prepare_submodule_repo_env(struct argv_array *out)
> DEFAULT_GIT_DIR_ENVIRONMENT);
> }
>
> -/* Helper function to display the submodule header line prior to the full
> - * summary output. If it can locate the submodule objects directory it will
> - * attempt to lookup both the left and right commits and put them into the
> - * left and right pointers.
> +/*
> + * Initialize 'out' based on the provided submodule path.
> + *
> + * Unlike repo_submodule_init, this tolerates submodules not present
> + * in .gitmodules. This function exists only to preserve historical behavior,
> + *
> + * Returns 0 on success, -1 when the submodule is not present.
> */
> -static void show_submodule_header(struct diff_options *o, const char *path,
> +static struct repository *open_submodule(const char *path)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + struct repository *out = xmalloc(sizeof(*out));
> +
> + if (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) {
> + strbuf_release(&sb);
> + free(out);
> + return NULL;
> + }
> +
> + out->submodule_prefix = xstrfmt("%s%s/",
> + the_repository->submodule_prefix ?
> + the_repository->submodule_prefix :
> + "", path);
> +
> + strbuf_release(&sb);
> + return out;
> +}
> +
> +/*
> + * Helper function to display the submodule header line prior to the full
> + * summary output.
> + *
> + * If it can locate the submodule git directory it will create a repository
> + * handle for the submodule and lookup both the left and right commits and
> + * put them into the left and right pointers.
> + */
> +static void show_submodule_header(struct diff_options *o,
> + const char *path,
> struct object_id *one, struct object_id *two,
> unsigned dirty_submodule,
> + struct repository *sub,
> struct commit **left, struct commit **right,
> struct commit_list **merge_bases)
> {
> @@ -507,7 +541,7 @@ static void show_submodule_header(struct diff_options *o, const char *path,
> else if (is_null_oid(two))
> message = "(submodule deleted)";
>
> - if (add_submodule_odb(path)) {
> + if (!sub) {
> if (!message)
> message = "(commits not present)";
> goto output_header;
> @@ -517,8 +551,8 @@ static void show_submodule_header(struct diff_options *o, const char *path,
> * Attempt to lookup the commit references, and determine if this is
> * a fast forward or fast backwards update.
> */
> - *left = lookup_commit_reference(the_repository, one);
> - *right = lookup_commit_reference(the_repository, two);
> + *left = lookup_commit_reference(sub, one);
> + *right = lookup_commit_reference(sub, two);
>
> /*
> * Warn about missing commits in the submodule project, but only if
> @@ -528,7 +562,7 @@ static void show_submodule_header(struct diff_options *o, const char *path,
> (!is_null_oid(two) && !*right))
> message = "(commits not present)";
>
> - *merge_bases = get_merge_bases(*left, *right);
> + *merge_bases = repo_get_merge_bases(sub, *left, *right);
> if (*merge_bases) {
> if ((*merge_bases)->item == *left)
> fast_forward = 1;
> @@ -562,16 +596,18 @@ void show_submodule_summary(struct diff_options *o, const char *path,
> struct rev_info rev;
> struct commit *left = NULL, *right = NULL;
> struct commit_list *merge_bases = NULL;
> + struct repository *sub;
>
> + sub = open_submodule(path);
> show_submodule_header(o, path, one, two, dirty_submodule,
> - &left, &right, &merge_bases);
> + sub, &left, &right, &merge_bases);
>
> /*
> * If we don't have both a left and a right pointer, there is no
> * reason to try and display a summary. The header line should contain
> * all the information the user needs.
> */
> - if (!left || !right)
> + if (!left || !right || !sub)
> goto out;
>
> /* Treat revision walker failure the same as missing commits */
> @@ -580,13 +616,17 @@ void show_submodule_summary(struct diff_options *o, const char *path,
> goto out;
> }
>
> - print_submodule_summary(&rev, o);
> + print_submodule_summary(sub, &rev, o);
>
> out:
> if (merge_bases)
> free_commit_list(merge_bases);
> clear_commit_marks(left, ~0);
> clear_commit_marks(right, ~0);
> + if (sub) {
> + repo_clear(sub);
> + free(sub);
> + }
> }
>
> void show_submodule_inline_diff(struct diff_options *o, const char *path,
> @@ -598,9 +638,11 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path,
> struct commit_list *merge_bases = NULL;
> struct child_process cp = CHILD_PROCESS_INIT;
> struct strbuf sb = STRBUF_INIT;
> + struct repository *sub;
>
> + sub = open_submodule(path);
> show_submodule_header(o, path, one, two, dirty_submodule,
> - &left, &right, &merge_bases);
> + sub, &left, &right, &merge_bases);
>
> /* We need a valid left and right commit to display a difference */
> if (!(left || is_null_oid(one)) ||
> @@ -661,6 +703,10 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path,
> clear_commit_marks(left, ~0);
> if (right)
> clear_commit_marks(right, ~0);
> + if (sub) {
> + repo_clear(sub);
> + free(sub);
> + }
> }
>
> int should_update_submodules(void)
next prev parent reply other threads:[~2018-11-02 13:03 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-30 22:07 [PATCHv2 00/24] Bring more repository handles into our code base] Stefan Beller
2018-10-30 22:07 ` [PATCH 01/24] Makefile: add pending semantic patches Stefan Beller
2018-10-31 6:42 ` Junio C Hamano
2018-11-08 20:52 ` [PATCH] " Stefan Beller
2018-11-09 4:55 ` Martin Ågren
2018-11-09 20:50 ` Stefan Beller
2018-11-09 5:18 ` Junio C Hamano
2018-11-09 21:58 ` Stefan Beller
2018-11-13 15:48 ` SZEDER Gábor
2018-11-14 4:15 ` Junio C Hamano
2018-11-10 0:10 ` [PATCH] coccicheck: introduce 'pending' " Stefan Beller
2018-11-10 20:37 ` Martin Ågren
2018-11-12 21:19 ` Josh Steadmon
2018-11-13 16:02 ` SZEDER Gábor
2018-10-30 22:07 ` [PATCH 02/24] sha1_file: allow read_object to read objects in arbitrary repositories Stefan Beller
2018-10-30 22:07 ` [PATCH 03/24] packfile: allow has_packed_and_bad to handle " Stefan Beller
2018-10-30 22:07 ` [PATCH 04/24] object-store: allow read_object_file_extended to read from " Stefan Beller
2018-10-30 22:07 ` [PATCH 05/24] object-store: prepare read_object_file to deal with " Stefan Beller
2018-10-30 22:07 ` [PATCH 06/24] object-store: prepare has_{sha1, object}_file[_with_flags] to handle " Stefan Beller
2018-10-30 22:08 ` [PATCH 07/24] object: parse_object to honor its repository argument Stefan Beller
2018-10-30 22:08 ` [PATCH 08/24] commit: allow parse_commit* to handle arbitrary repositories Stefan Beller
2018-10-30 22:08 ` [PATCH 09/24] commit-reach.c: allow paint_down_to_common " Stefan Beller
2018-10-30 22:08 ` [PATCH 10/24] commit-reach.c: allow merge_bases_many " Stefan Beller
2018-10-30 22:08 ` [PATCH 11/24] commit-reach.c: allow remove_redundant " Stefan Beller
2018-10-30 22:08 ` [PATCH 12/24] commit-reach.c: allow get_merge_bases_many_0 " Stefan Beller
2018-10-30 22:08 ` [PATCH 13/24] commit-reach: prepare get_merge_bases " Stefan Beller
2018-10-30 22:08 ` [PATCH 14/24] commit-reach: prepare in_merge_bases[_many] " Stefan Beller
2018-10-30 22:08 ` [PATCH 15/24] commit: prepare get_commit_buffer " Stefan Beller
2018-10-30 22:08 ` [PATCH 16/24] commit: prepare repo_unuse_commit_buffer " Stefan Beller
2018-10-30 22:08 ` [PATCH 17/24] commit: prepare logmsg_reencode " Stefan Beller
2018-10-30 22:08 ` [PATCH 18/24] pretty: prepare format_commit_message " Stefan Beller
2018-10-30 22:08 ` [PATCH 19/24] submodule: use submodule repos for object lookup Stefan Beller
2018-11-02 13:03 ` Derrick Stolee [this message]
2018-11-02 17:23 ` Stefan Beller
2018-11-02 17:27 ` Derrick Stolee
2018-10-30 22:08 ` [PATCH 20/24] submodule: don't add submodule as odb for push Stefan Beller
2018-10-30 22:08 ` [PATCH 21/24] commit-graph: convert remaining function to handle arbitrary repositories Stefan Beller
2018-10-30 22:08 ` [PATCH 22/24] commit: make free_commit_buffer and release_commit_memory repository agnostic Stefan Beller
2018-10-30 22:08 ` [PATCH 23/24] path.h: make REPO_GIT_PATH_FUNC " Stefan Beller
2018-10-30 22:08 ` [PATCH 24/24] t/helper/test-repository: celebrate independence from the_repository Stefan Beller
2018-10-31 6:41 ` [PATCHv2 00/24] Bring more repository handles into our code base] Junio C Hamano
2018-11-01 19:45 ` Stefan Beller
2018-11-02 2:00 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=127add9b-d58c-ee46-73de-41bfe915589b@gmail.com \
--to=stolee@gmail.com \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.com \
--cc=sbeller@google.com \
--cc=szeder.dev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).