Repository navigation
[Outreachy PATCH] t4041: use submodule repository for commit abbreviations - #2288
Open
poojarysrujith wants to merge 1 commit into
Open
poojarysrujith wants to merge 1 commit into
poojarysrujith wants to merge 1 commit into
Conversation
poojarysrujith
force-pushed
the
microproject-git-libification
branch
from
October 31, 2026 05:55
28abb2a to
7b2af1b
Compare
Author
|
/preview |
|
Preview email sent as pull.2288.git.1793426277131.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2288.git.1793435149942.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
poojarysrujith
force-pushed
the
microproject-git-libification
branch
from
October 31, 2026 18:05
7b2af1b to
3133cd3
Compare
Author
|
/submit |
|
Submitted as pull.2288.v2.git.1793473062113.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
poojarysrujith
force-pushed
the
microproject-git-libification
branch
from
October 31, 2026 19:20
3133cd3 to
a8c54b9
Compare
Author
|
/submit |
|
Submitted as pull.2288.v3.git.1793474657981.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Okhuomon Ajayi wrote on the Git mailing list (how to reply to this email): Hi Srujith,
I applied this on top of master and ran t4041. The new test fails
without the submodule.c change and passes with it, so it does catch
the problem.
A few small things:
On Sat, Oct 31, 2026 at 9:26 AM Srujith A Poojary via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Use the submodule repository when abbreviating commit IDs in
> submodule diff headers, so uniqueness is checked against the
> correct object database.
It would help to first describe what happens today: the commit IDs
in the "Submodule ..." header are abbreviated against the
superproject's objects, even though the commits live in the
submodule, so the abbreviation may not be unique there.
> [Outreachy PATCH] t4041: use submodule repository for commit
> abbreviations
The change itself is in submodule.c, so the title might read better
with that area, e.g. "submodule: use the submodule repository to
abbreviate commits".
> - strbuf_add_unique_abbrev(&sb, one, DEFAULT_ABBREV);
> + if (sub)
> + strbuf_repo_add_unique_abbrev(&sb, sub, one, DEFAULT_ABBREV);
> + else
> + strbuf_add_unique_abbrev(&sb, one, DEFAULT_ABBREV);
The same if/else appears twice. Maybe choose the repository once,
at the top of the function, and call strbuf_repo_add_unique_abbrev()
with it in both places. I also wonder whether the fallback could be
o->repo instead of the_repository, since the diff options already
carry a repository.
Thanks,
Okhuomon |
|
User |
|
Srujith Poojary wrote on the Git mailing list (how to reply to this email): On Sat, Oct 31, 2026 at 10:16 PM Okhuomon Ajayi
<okhuomonajayi54@gmail.com> wrote:
>
> Hi Srujith,
>
> I applied this on top of master and ran t4041. The new test fails
> without the submodule.c change and passes with it, so it does catch
> the problem.
>
> A few small things:
>
> On Sat, Oct 31, 2026 at 9:26 AM Srujith A Poojary via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > Use the submodule repository when abbreviating commit IDs in
> > submodule diff headers, so uniqueness is checked against the
> > correct object database.
>
> It would help to first describe what happens today: the commit IDs
> in the "Submodule ..." header are abbreviated against the
> superproject's objects, even though the commits live in the
> submodule, so the abbreviation may not be unique there.
>
> > [Outreachy PATCH] t4041: use submodule repository for commit
> > abbreviations
>
> The change itself is in submodule.c, so the title might read better
> with that area, e.g. "submodule: use the submodule repository to
> abbreviate commits".
>
> > - strbuf_add_unique_abbrev(&sb, one, DEFAULT_ABBREV);
> > + if (sub)
> > + strbuf_repo_add_unique_abbrev(&sb, sub, one, DEFAULT_ABBREV);
> > + else
> > + strbuf_add_unique_abbrev(&sb, one, DEFAULT_ABBREV);
>
> The same if/else appears twice. Maybe choose the repository once,
> at the top of the function, and call strbuf_repo_add_unique_abbrev()
> with it in both places. I also wonder whether the fallback could be
> o->repo instead of the_repository, since the diff options already
> carry a repository.
>
> Thanks,
> Okhuomon
Hi Okhuomon,
Thanks for reviewing the patch and testing the new test case.
I've made the suggested changes and submitted v2. I now use `sub` when
available and fall back to `o->repo` when it isn't. I also simplified
the code and updated the commit message.
Here is the updated patch:
https://lore.kernel.org/git/pull.2288.v2.git.1793473062113.gitgitgadget@gmail.com
Thanks again!
Regards,
Srujith |
|
Okhuomon Ajayi wrote on the Git mailing list (how to reply to this email): Hi Srujith,
On Sat, Oct 31, 2026 at 7:57 PM Srujith A Poojary via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> + struct repository *repo = sub ? sub : o->repo;
Thanks, v2 looks good to me.
> Submodule: use the submodule repository to abbreviate commits
One small nit: "git log --no-merges submodule.c" shows the area is
always written in lowercase ("submodule:"), so "submodule: use the
submodule repository to abbreviate commits" would match.
Thanks,
Okhuomon
On Sat, Oct 31, 2026 at 7:57 PM Srujith A Poojary via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Srujith A Poojary <poojarysrujith@gmail.com>
>
> The commit IDs shown in a submodule diff header belong to the
> submodule's object database. However, strbuf_add_unique_abbrev()
> uses the_repository, so it checks the superproject's object
> database when abbreviating those IDs.
>
> Use the submodule repository when it is available, and fall back
> to the repository from the diff options when it is not.
>
> Signed-off-by: Srujith A Poojary <poojarysrujith@gmail.com>
> ---
> [Outreachy PATCH] t4041: use submodule repository for commit
> abbreviations
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2288%2Fpoojarysrujith%2Fmicroproject-git-libification-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2288/poojarysrujith/microproject-git-libification-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/2288
>
> Range-diff vs v1:
>
> 1: 7b2af1b46a ! 1: 3133cd3017 [Outreachy PATCH] t4041: use submodule repository for commit abbreviations
> @@ Metadata
> Author: Srujith A Poojary <poojarysrujith@gmail.com>
>
> ## Commit message ##
> - [Outreachy PATCH] t4041: use submodule repository for commit abbreviations
> + Submodule: use the submodule repository to abbreviate commits
>
> - Use the submodule repository when abbreviating commit IDs in
> - submodule diff headers, so uniqueness is checked against the
> - correct object database.
> + The commit IDs shown in a submodule diff header belong to the
> + submodule's object database. However, strbuf_add_unique_abbrev()
> + uses the_repository, so it checks the superproject's object
> + database when abbreviating those IDs.
> +
> + Use the submodule repository when it is available, and fall back
> + to the repository from the diff options when it is not.
>
> Signed-off-by: Srujith A Poojary <poojarysrujith@gmail.com>
>
> ## submodule.c ##
> @@ submodule.c: static void show_submodule_header(struct diff_options *o,
> + struct commit **left, struct commit **right,
> + struct commit_list **merge_bases)
> + {
> ++ struct repository *repo = sub ? sub : o->repo;
> + const char *message = NULL;
> + struct strbuf sb = STRBUF_INIT;
> + int fast_forward = 0, fast_backward = 0;
> +@@ submodule.c: static void show_submodule_header(struct diff_options *o,
>
> output_header:
> strbuf_addf(&sb, "Submodule %s ", path);
> - strbuf_add_unique_abbrev(&sb, one, DEFAULT_ABBREV);
> -+ if (sub)
> -+ strbuf_repo_add_unique_abbrev(&sb, sub, one, DEFAULT_ABBREV);
> -+ else
> -+ strbuf_add_unique_abbrev(&sb, one, DEFAULT_ABBREV);
> ++ strbuf_repo_add_unique_abbrev(&sb, repo, one, DEFAULT_ABBREV);
> strbuf_addstr(&sb, (fast_backward || fast_forward) ? ".." : "...");
> - strbuf_add_unique_abbrev(&sb, two, DEFAULT_ABBREV);
> -+ if (sub)
> -+ strbuf_repo_add_unique_abbrev(&sb, sub, two, DEFAULT_ABBREV);
> -+ else
> -+ strbuf_add_unique_abbrev(&sb, two, DEFAULT_ABBREV);
> ++ strbuf_repo_add_unique_abbrev(&sb, repo, two, DEFAULT_ABBREV);
> if (message)
> strbuf_addf(&sb, " %s\n", message);
> else
>
>
> submodule.c | 5 +++--
> t/t4041-diff-submodule-option.sh | 18 ++++++++++++++++++
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 6fcb606f7e..1302c11ddf 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -543,6 +543,7 @@ static void show_submodule_header(struct diff_options *o,
> struct commit **left, struct commit **right,
> struct commit_list **merge_bases)
> {
> + struct repository *repo = sub ? sub : o->repo;
> const char *message = NULL;
> struct strbuf sb = STRBUF_INIT;
> int fast_forward = 0, fast_backward = 0;
> @@ -599,9 +600,9 @@ static void show_submodule_header(struct diff_options *o,
>
> output_header:
> strbuf_addf(&sb, "Submodule %s ", path);
> - strbuf_add_unique_abbrev(&sb, one, DEFAULT_ABBREV);
> + strbuf_repo_add_unique_abbrev(&sb, repo, one, DEFAULT_ABBREV);
> strbuf_addstr(&sb, (fast_backward || fast_forward) ? ".." : "...");
> - strbuf_add_unique_abbrev(&sb, two, DEFAULT_ABBREV);
> + strbuf_repo_add_unique_abbrev(&sb, repo, two, DEFAULT_ABBREV);
> if (message)
> strbuf_addf(&sb, " %s\n", message);
> else
> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> index 4dd4954260..e80472af32 100755
> --- a/t/t4041-diff-submodule-option.sh
> +++ b/t/t4041-diff-submodule-option.sh
> @@ -14,6 +14,24 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> . ./test-lib.sh
>
> +test_expect_success SHA1 'submodule commit abbreviation uses submodule object database' '
> + test_when_finished "rm -rf collision-super" &&
> + git init -q collision-super &&
> + git init -q collision-super/sm1 &&
> + printf "0\n1\n2\n3\n4\n5\n6\n7\n8\n9\n\nb1rwzyc3\n" >collision-super/sm1/a0blgqsjc &&
> + git -C collision-super/sm1 add a0blgqsjc &&
> + tree=$(git -C collision-super/sm1 write-tree) &&
> + commit1=$(GIT_AUTHOR_DATE="1112911993 -0700" GIT_COMMITTER_DATE="1112911993 -0700" git -C collision-super/sm1 commit-tree "$tree" -m a2onsxbvj) &&
> + commit2=$(printf "child\n" | GIT_AUTHOR_DATE="1112912053 -0700" GIT_COMMITTER_DATE="1112912053 -0700" git -C collision-super/sm1 commit-tree "$tree" -p "$commit1") &&
> + git -C collision-super/sm1 checkout -q "$commit2" &&
> + git -C collision-super update-index --add --cacheinfo 160000,"$commit1",sm1 &&
> + git -C collision-super commit -q -m "Add submodule" &&
> + git -C collision-super diff --submodule=log >actual &&
> + printf "Submodule sm1 0000000000e..%s:\n > child\n" \
> + "$(git -C collision-super/sm1 rev-parse --short "$commit2")" >expected &&
> + test_cmp expected actual
> +'
> +
> # Test non-UTF-8 encoding in case iconv is available.
> if test_have_prereq ICONV
> then
>
> base-commit: 50fe5cc63f9cd6e89b87a2c9ccf96a444f4cfdb7
> --
> gitgitgadget
> |
|
Srujith Poojary wrote on the Git mailing list (how to reply to this email): On Sun, Nov 1, 2026 at 12:39 AM Okhuomon Ajayi
<okhuomonajayi54@gmail.com> wrote:
>
> Hi Srujith,
>
> On Sat, Oct 31, 2026 at 7:57 PM Srujith A Poojary via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > + struct repository *repo = sub ? sub : o->repo;
>
> Thanks, v2 looks good to me.
>
> > Submodule: use the submodule repository to abbreviate commits
>
> One small nit: "git log --no-merges submodule.c" shows the area is
> always written in lowercase ("submodule:"), so "submodule: use the
> submodule repository to abbreviate commits" would match.
>
> Thanks,
> Okhuomon
>
> On Sat, Oct 31, 2026 at 7:57 PM Srujith A Poojary via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Srujith A Poojary <poojarysrujith@gmail.com>
> >
> > The commit IDs shown in a submodule diff header belong to the
> > submodule's object database. However, strbuf_add_unique_abbrev()
> > uses the_repository, so it checks the superproject's object
> > database when abbreviating those IDs.
> >
> > Use the submodule repository when it is available, and fall back
> > to the repository from the diff options when it is not.
> >
> > Signed-off-by: Srujith A Poojary <poojarysrujith@gmail.com>
> > ---
> > [Outreachy PATCH] t4041: use submodule repository for commit
> > abbreviations
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2288%2Fpoojarysrujith%2Fmicroproject-git-libification-v2
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2288/poojarysrujith/microproject-git-libification-v2
> > Pull-Request: https://github.com/gitgitgadget/git/pull/2288
> >
> > Range-diff vs v1:
> >
> > 1: 7b2af1b46a ! 1: 3133cd3017 [Outreachy PATCH] t4041: use submodule repository for commit abbreviations
> > @@ Metadata
> > Author: Srujith A Poojary <poojarysrujith@gmail.com>
> >
> > ## Commit message ##
> > - [Outreachy PATCH] t4041: use submodule repository for commit abbreviations
> > + Submodule: use the submodule repository to abbreviate commits
> >
> > - Use the submodule repository when abbreviating commit IDs in
> > - submodule diff headers, so uniqueness is checked against the
> > - correct object database.
> > + The commit IDs shown in a submodule diff header belong to the
> > + submodule's object database. However, strbuf_add_unique_abbrev()
> > + uses the_repository, so it checks the superproject's object
> > + database when abbreviating those IDs.
> > +
> > + Use the submodule repository when it is available, and fall back
> > + to the repository from the diff options when it is not.
> >
> > Signed-off-by: Srujith A Poojary <poojarysrujith@gmail.com>
> >
> > ## submodule.c ##
> > @@ submodule.c: static void show_submodule_header(struct diff_options *o,
> > + struct commit **left, struct commit **right,
> > + struct commit_list **merge_bases)
> > + {
> > ++ struct repository *repo = sub ? sub : o->repo;
> > + const char *message = NULL;
> > + struct strbuf sb = STRBUF_INIT;
> > + int fast_forward = 0, fast_backward = 0;
> > +@@ submodule.c: static void show_submodule_header(struct diff_options *o,
> >
> > output_header:
> > strbuf_addf(&sb, "Submodule %s ", path);
> > - strbuf_add_unique_abbrev(&sb, one, DEFAULT_ABBREV);
> > -+ if (sub)
> > -+ strbuf_repo_add_unique_abbrev(&sb, sub, one, DEFAULT_ABBREV);
> > -+ else
> > -+ strbuf_add_unique_abbrev(&sb, one, DEFAULT_ABBREV);
> > ++ strbuf_repo_add_unique_abbrev(&sb, repo, one, DEFAULT_ABBREV);
> > strbuf_addstr(&sb, (fast_backward || fast_forward) ? ".." : "...");
> > - strbuf_add_unique_abbrev(&sb, two, DEFAULT_ABBREV);
> > -+ if (sub)
> > -+ strbuf_repo_add_unique_abbrev(&sb, sub, two, DEFAULT_ABBREV);
> > -+ else
> > -+ strbuf_add_unique_abbrev(&sb, two, DEFAULT_ABBREV);
> > ++ strbuf_repo_add_unique_abbrev(&sb, repo, two, DEFAULT_ABBREV);
> > if (message)
> > strbuf_addf(&sb, " %s\n", message);
> > else
> >
> >
> > submodule.c | 5 +++--
> > t/t4041-diff-submodule-option.sh | 18 ++++++++++++++++++
> > 2 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/submodule.c b/submodule.c
> > index 6fcb606f7e..1302c11ddf 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -543,6 +543,7 @@ static void show_submodule_header(struct diff_options *o,
> > struct commit **left, struct commit **right,
> > struct commit_list **merge_bases)
> > {
> > + struct repository *repo = sub ? sub : o->repo;
> > const char *message = NULL;
> > struct strbuf sb = STRBUF_INIT;
> > int fast_forward = 0, fast_backward = 0;
> > @@ -599,9 +600,9 @@ static void show_submodule_header(struct diff_options *o,
> >
> > output_header:
> > strbuf_addf(&sb, "Submodule %s ", path);
> > - strbuf_add_unique_abbrev(&sb, one, DEFAULT_ABBREV);
> > + strbuf_repo_add_unique_abbrev(&sb, repo, one, DEFAULT_ABBREV);
> > strbuf_addstr(&sb, (fast_backward || fast_forward) ? ".." : "...");
> > - strbuf_add_unique_abbrev(&sb, two, DEFAULT_ABBREV);
> > + strbuf_repo_add_unique_abbrev(&sb, repo, two, DEFAULT_ABBREV);
> > if (message)
> > strbuf_addf(&sb, " %s\n", message);
> > else
> > diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> > index 4dd4954260..e80472af32 100755
> > --- a/t/t4041-diff-submodule-option.sh
> > +++ b/t/t4041-diff-submodule-option.sh
> > @@ -14,6 +14,24 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> >
> > . ./test-lib.sh
> >
> > +test_expect_success SHA1 'submodule commit abbreviation uses submodule object database' '
> > + test_when_finished "rm -rf collision-super" &&
> > + git init -q collision-super &&
> > + git init -q collision-super/sm1 &&
> > + printf "0\n1\n2\n3\n4\n5\n6\n7\n8\n9\n\nb1rwzyc3\n" >collision-super/sm1/a0blgqsjc &&
> > + git -C collision-super/sm1 add a0blgqsjc &&
> > + tree=$(git -C collision-super/sm1 write-tree) &&
> > + commit1=$(GIT_AUTHOR_DATE="1112911993 -0700" GIT_COMMITTER_DATE="1112911993 -0700" git -C collision-super/sm1 commit-tree "$tree" -m a2onsxbvj) &&
> > + commit2=$(printf "child\n" | GIT_AUTHOR_DATE="1112912053 -0700" GIT_COMMITTER_DATE="1112912053 -0700" git -C collision-super/sm1 commit-tree "$tree" -p "$commit1") &&
> > + git -C collision-super/sm1 checkout -q "$commit2" &&
> > + git -C collision-super update-index --add --cacheinfo 160000,"$commit1",sm1 &&
> > + git -C collision-super commit -q -m "Add submodule" &&
> > + git -C collision-super diff --submodule=log >actual &&
> > + printf "Submodule sm1 0000000000e..%s:\n > child\n" \
> > + "$(git -C collision-super/sm1 rev-parse --short "$commit2")" >expected &&
> > + test_cmp expected actual
> > +'
> > +
> > # Test non-UTF-8 encoding in case iconv is available.
> > if test_have_prereq ICONV
> > then
> >
> > base-commit: 50fe5cc63f9cd6e89b87a2c9ccf96a444f4cfdb7
> > --
> > gitgitgadget
Hi Okhuomon,
Thanks for pointing that out. I've corrected the subject to use
lowercase `submodule` and submitted v3.
Regards,
Srujith
> > |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Srujith A Poojary via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Srujith A Poojary <poojarysrujith@gmail.com>
>
> The commit IDs shown in a submodule diff header belong to the
> submodule's object database. However, strbuf_add_unique_abbrev()
> uses the_repository, so it checks the superproject's object
> database when abbreviating those IDs.
>
> Use the submodule repository when it is available, and fall back
> to the repository from the diff options when it is not.
While the fallback behaviour does not make much sense (the commit
object recorded in gitlink is not expected to be found in the
superproject's repository, which is the reason why you are adding
this fix in the first place), it would mean that for unpopulated
submodules for which you haven't run "git submodule init", the
behaviour would stay the ame as before.
> diff --git a/submodule.c b/submodule.c
> index 6fcb606f7e..1302c11ddf 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -543,6 +543,7 @@ static void show_submodule_header(struct diff_options *o,
> struct commit **left, struct commit **right,
> struct commit_list **merge_bases)
> {
> + struct repository *repo = sub ? sub : o->repo;
> const char *message = NULL;
> struct strbuf sb = STRBUF_INIT;
> int fast_forward = 0, fast_backward = 0;
> @@ -599,9 +600,9 @@ static void show_submodule_header(struct diff_options *o,
>
> output_header:
> strbuf_addf(&sb, "Submodule %s ", path);
> - strbuf_add_unique_abbrev(&sb, one, DEFAULT_ABBREV);
> + strbuf_repo_add_unique_abbrev(&sb, repo, one, DEFAULT_ABBREV);
> strbuf_addstr(&sb, (fast_backward || fast_forward) ? ".." : "...");
> - strbuf_add_unique_abbrev(&sb, two, DEFAULT_ABBREV);
> + strbuf_repo_add_unique_abbrev(&sb, repo, two, DEFAULT_ABBREV);
> if (message)
> strbuf_addf(&sb, " %s\n", message);
> else
Quite straightforward.
> +test_expect_success SHA1 'submodule commit abbreviation uses submodule object database' '
Hmph. I wonder if there is a way to use the test_oid_cache
facility to allow test with both SHA-1 and SHA-256 hashes?
Will queue.
Thanks. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@6db9c46. |
|
There are issues in commit 02cde80:
|
poojarysrujith
force-pushed
the
microproject-git-libification
branch
from
November 2, 2026 19:00
02cde80 to
99a1234
Compare
Author
|
/submit |
|
Submitted as pull.2288.v4.git.1793648430.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -543,6 +543,7 @@ static void show_submodule_header(struct diff_options *o, | |||
| struct commit **left, struct commit **right, | |||
There was a problem hiding this comment.
Srujith Poojary wrote on the Git mailing list (how to reply to this email):
On Tue, Nov 3, 2026 at 1:10 AM Srujith A Poojary via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Srujith A Poojary <poojarysrujith@gmail.com>
>
> The commit IDs shown in a submodule diff header belong to the
> submodule's object database. However, strbuf_add_unique_abbrev()
> uses the_repository, so it checks the superproject's object
> database when abbreviating those IDs.
>
> Use the submodule repository when it is available, and fall back
> to the repository from the diff options when it is not.
>
> Signed-off-by: Srujith A Poojary <poojarysrujith@gmail.com>
> ---
> submodule.c | 5 +++--
> t/t4041-diff-submodule-option.sh | 18 ++++++++++++++++++
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 6fcb606f7e..1302c11ddf 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -543,6 +543,7 @@ static void show_submodule_header(struct diff_options *o,
> struct commit **left, struct commit **right,
> struct commit_list **merge_bases)
> {
> + struct repository *repo = sub ? sub : o->repo;
> const char *message = NULL;
> struct strbuf sb = STRBUF_INIT;
> int fast_forward = 0, fast_backward = 0;
> @@ -599,9 +600,9 @@ static void show_submodule_header(struct diff_options *o,
>
> output_header:
> strbuf_addf(&sb, "Submodule %s ", path);
> - strbuf_add_unique_abbrev(&sb, one, DEFAULT_ABBREV);
> + strbuf_repo_add_unique_abbrev(&sb, repo, one, DEFAULT_ABBREV);
> strbuf_addstr(&sb, (fast_backward || fast_forward) ? ".." : "...");
> - strbuf_add_unique_abbrev(&sb, two, DEFAULT_ABBREV);
> + strbuf_repo_add_unique_abbrev(&sb, repo, two, DEFAULT_ABBREV);
> if (message)
> strbuf_addf(&sb, " %s\n", message);
> else
> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> index 4dd4954260..e80472af32 100755
> --- a/t/t4041-diff-submodule-option.sh
> +++ b/t/t4041-diff-submodule-option.sh
> @@ -14,6 +14,24 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> . ./test-lib.sh
>
> +test_expect_success SHA1 'submodule commit abbreviation uses submodule object database' '
> + test_when_finished "rm -rf collision-super" &&
> + git init -q collision-super &&
> + git init -q collision-super/sm1 &&
> + printf "0\n1\n2\n3\n4\n5\n6\n7\n8\n9\n\nb1rwzyc3\n" >collision-super/sm1/a0blgqsjc &&
> + git -C collision-super/sm1 add a0blgqsjc &&
> + tree=$(git -C collision-super/sm1 write-tree) &&
> + commit1=$(GIT_AUTHOR_DATE="1112911993 -0700" GIT_COMMITTER_DATE="1112911993 -0700" git -C collision-super/sm1 commit-tree "$tree" -m a2onsxbvj) &&
> + commit2=$(printf "child\n" | GIT_AUTHOR_DATE="1112912053 -0700" GIT_COMMITTER_DATE="1112912053 -0700" git -C collision-super/sm1 commit-tree "$tree" -p "$commit1") &&
> + git -C collision-super/sm1 checkout -q "$commit2" &&
> + git -C collision-super update-index --add --cacheinfo 160000,"$commit1",sm1 &&
> + git -C collision-super commit -q -m "Add submodule" &&
> + git -C collision-super diff --submodule=log >actual &&
> + printf "Submodule sm1 0000000000e..%s:\n > child\n" \
> + "$(git -C collision-super/sm1 rev-parse --short "$commit2")" >expected &&
> + test_cmp expected actual
> +'
> +
> # Test non-UTF-8 encoding in case iconv is available.
> if test_have_prereq ICONV
> then
> --
> gitgitgadget
>
Hi Junio,
Thanks for the suggestion.
I've updated the t4041 test to use `test_oid_cache` with separate
expected abbreviations for SHA-1 and SHA-256. I've also removed the
SHA1-only prerequisite so that the test runs with both hash
algorithms.
I tested the updated test script using both configurations:
- SHA-1: all 48 tests passed.
- SHA-256: all 48 tests passed.
I've updated the commit message, added the Signed-off-by trailer, and
submitted the revised v4 patch series.
The updated patch series is available here:
https://lore.kernel.org/git/pull.2288.v4.git.1793648490.gitgitgadget@gmail.com/
Thanks for the suggestion and review.
Regards,
Srujith
|
There was a status update in the "New Topics" section about the branch The abbreviation of commit IDs in submodule diff headers has been corrected to use the submodule's object database instead of the superproject's, avoiding potential ambiguity. Needs review. source: <pull.2288.v4.git.1793648430.gitgitgadget@gmail.com> |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Srujith A Poojary via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> Srujith A Poojary (2):
> submodule: use the submodule repository to abbreviate commits
> t4041: test SHA-256 submodule abbreviations
If you are sending a cover letter, you are expected to summarize the
changes since the previous iteration(s) in it, so that those who are
joining the discussion in the middle can tell the history behind the
current iteration.
> submodule.c | 5 +++--
> t/t4041-diff-submodule-option.sh | 23 +++++++++++++++++++++++
> 2 files changed, 26 insertions(+), 2 deletions(-)
>
>
> base-commit: 50fe5cc63f9cd6e89b87a2c9ccf96a444f4cfdb7
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2288%2Fpoojarysrujith%2Fmicroproject-git-libification-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2288/poojarysrujith/microproject-git-libification-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/2288
>
> Range-diff vs v3:
>
> 1: a8c54b9292 = 1: a8c54b9292 submodule: use the submodule repository to abbreviate commits
> -: ---------- > 2: 99a1234544 t4041: test SHA-256 submodule abbreviations |
| @@ -14,6 +14,29 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME | |||
|
|
|||
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Srujith A Poojary via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Srujith A Poojary <poojarysrujith@gmail.com>
>
> The expected abbreviation for a submodule commit depends on the
> repository's hash algorithm. Use test_oid_cache to provide the
> expected value for each algorithm and remove the SHA1 prerequisite
> so this test runs with both SHA-1 and SHA-256.
>
> Signed-off-by: Srujith A Poojary <poojarysrujith@gmail.com>
> ---
> t/t4041-diff-submodule-option.sh | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
It is strange to test SHA-1 in 1/2 and add test for SHA-256 as if an
afterthought in 2/2. Shouldn't all of these come in a single patch?
The change to add the second hash algorithm support is surprisingly
clean, primarily thanks to how test_oid* support was done long ago.
> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> index e80472af32..4539d6bba5 100755
> --- a/t/t4041-diff-submodule-option.sh
> +++ b/t/t4041-diff-submodule-option.sh
> @@ -14,7 +14,11 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> . ./test-lib.sh
>
> -test_expect_success SHA1 'submodule commit abbreviation uses submodule object database' '
> +test_expect_success 'submodule commit abbreviation uses submodule object database' '
> + test_oid_cache <<-EOF &&
> + submodule_commit1_abbrev sha1:0000000000e
> + submodule_commit1_abbrev sha256:24c81a5
> + EOF
> test_when_finished "rm -rf collision-super" &&
> git init -q collision-super &&
> git init -q collision-super/sm1 &&
> @@ -27,7 +31,8 @@ test_expect_success SHA1 'submodule commit abbreviation uses submodule object da
> git -C collision-super update-index --add --cacheinfo 160000,"$commit1",sm1 &&
> git -C collision-super commit -q -m "Add submodule" &&
> git -C collision-super diff --submodule=log >actual &&
> - printf "Submodule sm1 0000000000e..%s:\n > child\n" \
> + printf "Submodule sm1 %s..%s:\n > child\n" \
> + "$(test_oid submodule_commit1_abbrev)" \
> "$(git -C collision-super/sm1 rev-parse --short "$commit2")" >expected &&
> test_cmp expected actual
> 'The commit IDs shown in a submodule diff header belong to the submodule's object database. However, strbuf_add_unique_abbrev() uses the_repository, so it checks the superproject's object database when abbreviating those IDs. Use the submodule repository when it is available, and fall back to the repository from the diff options when it is not. The expected abbreviation for a submodule commit depends on the repository's hash algorithm. Use test_oid_cache to provide the expected value for each algorithm and remove the SHA1 prerequisite so this test runs with both SHA-1 and SHA-256. Signed-off-by: Srujith A Poojary <poojarysrujith@gmail.com>
poojarysrujith
force-pushed
the
microproject-git-libification
branch
from
November 4, 2026 03:19
99a1234 to
f94aa24
Compare
Author
|
/preview |
|
Preview email sent as pull.2288.v5.git.1793762523507.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2288.v5.git.1793763781129.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cc: Okhuomon Ajayi okhuomonajayi54@gmail.com