Git development
 help / color / mirror / Atom feed
* Re: [PATCH] doc/cat-file: clarify description regarding various command forms
From: Jeff King @ 2023-10-03 20:06 UTC (permalink / raw)
  To: Štěpán Němec; +Cc: git, avarab
In-Reply-To: <20231003082513.3003520-1-stepnem@smrk.net>

On Tue, Oct 03, 2023 at 10:25:13AM +0200, Štěpán Němec wrote:

> The DESCRIPTION's "first form" is actually the 1st, 2nd, 3rd and 5th
> form in SYNOPSIS, the "second form" is the 4th one.
> 
> Interestingly, this state of affairs was introduced in
> 97fe7250753b (cat-file docs: fix SYNOPSIS and "-h" output, 2021-12-28)
> with the claim of "Now the two will match again." ("the two" being
> DESCRIPTION and SYNOPSIS)...
> 
> Ordinals are hard, let's try talking about batch and non-batch mode
> instead.

Thanks, I think this is a good direction. Two things I noticed:

>  DESCRIPTION
>  -----------
> -In its first form, the command provides the content or the type of an object in
> +This command can operate in two modes, depending on whether an option from
> +the `--batch` family is specified.
> +
> +In non-batch mode, the command provides the content or the type of an object in
>  the repository. The type is required unless `-t` or `-p` is used to find the
>  object type, or `-s` is used to find the object size, or `--textconv` or
>  `--filters` is used (which imply type "blob").

The existing text here is already a bit vague, considering the number of
operations it covers (like "-e", for example, which is not showing "the
content or the type" at all). But that is not new in your patch, and it
is maybe even OK to be a bit vague here, and let the OPTIONS section
cover the specifics.

> -In the second form, a list of objects (separated by linefeeds) is provided on
> +In batch mode, a list of objects (separated by linefeeds) is provided on
>  stdin, and the SHA-1, type, and size of each object is printed on stdout. The
>  output format can be overridden using the optional `<format>` argument. If
>  either `--textconv` or `--filters` was specified, the input is expected to

I think this got a bit inaccurate with "--batch-command", which is a
whole different mode itself compared to --batch and --batch-check. I
don't think your patch is really making anything worse, but arguably
there are three "major modes" here.

-Peff

^ permalink raw reply

* Re: Possible to update-ref remote repository?
From: Jeff King @ 2023-10-03 20:00 UTC (permalink / raw)
  To: Jesse Hopkins; +Cc: Junio C Hamano, git
In-Reply-To: <CAL3By-_jrYDenCc8HSrtnR3cZD19z7bvwVOOoO4XG+6aNFxyeQ@mail.gmail.com>

On Sun, Oct 01, 2023 at 04:03:29PM -0600, Jesse Hopkins wrote:

> Thanks for the reply.  Would you be able to point me to some
> breadcrumbs for the "required protocol messages"?  I might try to
> tinker in some spare time.

Try "git help protocol-pack" in a recent version of Git (this used to be
in Documentation/technical/ of the repository, but much of that content
was moved into manpages around the v2.38 timeframe).

For a local or ssh connection, I think it is as simple as:

  # you somehow happen to know this commit exists on the server,
  # and what the current value of the ref is. If you don't know the
  # current value, you can pull it from receive-pack's ref
  # advertisement (I'll leave that as an exercise for the reader).
  old=1234abcd...
  new=5678cdef...
  ref=refs/heads/main

  # we'll use a local repository here, but you can replace receive-pack
  # invocation below with with "ssh $host git receive-pack $repo"
  repo=/path/to/repo.git

  {
    # git's pkt-line format is a 4-byte header with the ascii hex size of
    # the packet, followed by N-4 bytes of data. Each ref update is
    # in its own pkt, but we have just one.
    cmd="$old $new $ref"
    printf "%04x%s" $((${#cmd} + 4)) "$cmd"

    # An all-zero flush packet indicates the end of the list of updates.
    printf "0000"

    # the server insists that we send a valid packfile, even if it is
    # empty. This is from "git help format-pack" (the section on .pack
    # files), though you could also generate it with "git pack-objects
    # --stdout </dev/null".
    printf 'PACK' ;# packfile
    printf '\0\0\0\2' ;# version 2
    printf '\0\0\0\0' ;# zero objects
    # checksum, which is the sha1 of the rest of the pack
    printf "\2\235\10\202\73\330\250\352\265\20\255\152\307\134\202\74\375\76\323\36"
  } |
  git receive-pack "$repo"

You can get fancier by specifying capabilities (you might want
"report-status", for example).

That will work for local or ssh repos. For http, it gets a little more
complicated. See the section "smart service git-receive-pack" of "git
help protocol-http".

All that said, I do think it might be reasonable for git-push to support
this directly. It is basically:

  1. Let the command run in a non-repo, skipping anything that requires
     it. This _might_ be a maintenance headache, but as a fallback you
     could always run from an empty local repository.

  2. Tell it to always generate an empty pack (basically, a "trust me,
     the other side will be OK with it" option).

The second part looks like something like this:

diff --git a/send-pack.c b/send-pack.c
index 89aca9d829..c54463c181 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -58,6 +58,9 @@ static void feed_object(const struct object_id *oid, FILE *fh, int negative)
 	putc('\n', fh);
 }
 
+/* obviously this should be passed down somehow in a real patch */
+#define SPECIAL_EMPTY_PACK_OPTION 1
+
 /*
  * Make a pack stream and spit it out into file descriptor fd
  */
@@ -103,17 +106,19 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,
 	 * parameters by writing to the pipe.
 	 */
 	po_in = xfdopen(po.in, "w");
-	for (i = 0; i < advertised->nr; i++)
-		feed_object(&advertised->oid[i], po_in, 1);
-	for (i = 0; i < negotiated->nr; i++)
-		feed_object(&negotiated->oid[i], po_in, 1);
-
-	while (refs) {
-		if (!is_null_oid(&refs->old_oid))
-			feed_object(&refs->old_oid, po_in, 1);
-		if (!is_null_oid(&refs->new_oid))
-			feed_object(&refs->new_oid, po_in, 0);
-		refs = refs->next;
+	if (!SPECIAL_EMPTY_PACK_OPTION) {
+		for (i = 0; i < advertised->nr; i++)
+			feed_object(&advertised->oid[i], po_in, 1);
+		for (i = 0; i < negotiated->nr; i++)
+			feed_object(&negotiated->oid[i], po_in, 1);
+
+		while (refs) {
+			if (!is_null_oid(&refs->old_oid))
+				feed_object(&refs->old_oid, po_in, 1);
+			if (!is_null_oid(&refs->new_oid))
+				feed_object(&refs->new_oid, po_in, 0);
+			refs = refs->next;
+		}
 	}
 
 	fflush(po_in);

Come to think of it, you could probably fake it by wrapping
git-pack-objects with a script that throws away its input. Maybe hard to
do because its a builtin (and we run it as "git pack-objects", which
executes it directly in-process).

-Peff

^ permalink raw reply related

* Re: [silly] loose, pack, and another thing?
From: Jeff King @ 2023-10-03 19:09 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: Junio C Hamano, git
In-Reply-To: <20230928214010.3502838-1-jonathantanmy@google.com>

On Thu, Sep 28, 2023 at 02:40:10PM -0700, Jonathan Tan wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> > Just wondering if it would help to have the third kind of object
> > representation in the object database, sitting next to loose objects
> > and packed objects, say .git/objects/verbatim/<hex-object-name> for
> > the contents and .git/objects/verbatim/<hex-object-name>.type that
> > records "blob", "tree", "commit", or "tag" (in practice, I would
> > expect huge "blob" objects would be the only ones that use this
> > mechanism).
> > 
> > The contents will be stored verbatim without compression and without
> > any object header (i.e., the usual "<type> <length>\0") and the file
> > could be "ln"ed (or "cow"ed if the underlying filesystem allows it)
> > to materialize it in the working tree if needed.
> 
> This sounds like a useful feature. We probably would want to use the
> "ln" or "cow" every time we use streaming (stream_blob_to_fd() in
> streaming.h) currently, so hopefully we won't need to increase the
> number of ways in which we can write an object to the worktree (just
> change the streaming to write to a filename instead of an fd).

One thing that scares me about a regular "ln" between the worktree and
odb is that you are very susceptible to corrupting the repository by
modifying the worktree file with regular tools. If they do a complete
rewrite and atomic rename (or link) to put the new file in place, that
is OK. But opening the file for appending, or general writing, is bad.

You can get some safety with the immutable attribute (which applies to
the inode itself, and thus any path that hardlinks to it). But setting
that usually requires being root. And it creates other irritations for
normal use (you have to unset it before even removing the hardlink).

It would be nice if there was some portable copy-on-write abstraction we
could rely on, but I don't think there is one.

-Peff

^ permalink raw reply

* Re: [Outreachy] Move existing tests to a unit testing framework
From: Junio C Hamano @ 2023-10-03 18:51 UTC (permalink / raw)
  To: Luma; +Cc: git
In-Reply-To: <CAFR+8DynAJ7eieMYUrezoNii5tzARNbESFxRCcT4w6okS5FZDg@mail.gmail.com>

Luma <ach.lumap@gmail.com> writes:

> Hi;
> My name is Luma, and  I wanted to take a moment to introduce myself
> and share some
> insights on an essential aspect of  avoiding pipes in git related
> commands in test scripts.
>
> I am an outreachy applicant for the December 2023 cohort and look
> forward to learning from you.

I notice that the title of the message and the immediate topic you
discuss in the body of the message do not match.  I presume that the
topic on the title is what you prefer to work on if the unit testing
framework is ready by the time Outreachy program starts, and the
mention about "do not clobber exit code of Git with pipes in the
tests" is your "dip the tip of a toe in water" microproject?

Welcome to the Git development community.

Do you have a single word name?  If so please disregard the below,
but in case "Luma" is just a nickname (e.g. like I am introducing
myself to my Git friends "Hi, I am Gitster!") you use online, please
read on.

For signing off your patches, we'd prefer to see your real name
used, as Signed-off-by: is meant to have legal significance.  And
because we also expect the authorship identity to match the
name/e-mail of the sign-off, it would mean your patch submissions
are expected to look like:

	From: Luma <ach.lumap@gmail.com>
	Subject: ... title of the patch goes here ...

	... body of the proposed commit log message goes here...

	Signed-off-by: Luma <ach.lumap@gmail.com>

but "Luma" replaced with your full real name.

Thanks.

^ permalink raw reply

* [PATCH 6/6] t7420: Test that we correctly handle renamed submodules
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
  To: git; +Cc: Jan Alexander Steffens (heftig)
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>

Create a second submodule with a name that differs from its path. Test
that calling set-url modifies the correct .gitmodules entries. Make sure
we don't create a section named after the path instead of the name.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 t/t7420-submodule-set-url.sh | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh
index aa63d806fe..bf7f15ee79 100755
--- a/t/t7420-submodule-set-url.sh
+++ b/t/t7420-submodule-set-url.sh
@@ -25,34 +25,56 @@ test_expect_success 'submodule config cache setup' '
 		git add file &&
 		git commit -ma
 	) &&
+	mkdir namedsubmodule &&
+	(
+		cd namedsubmodule &&
+		git init &&
+		echo 1 >file &&
+		git add file &&
+		git commit -m1
+	) &&
 	mkdir super &&
 	(
 		cd super &&
 		git init &&
 		git submodule add ../submodule &&
-		git commit -m "add submodule"
+		git submodule add --name thename ../namedsubmodule thepath &&
+		git commit -m "add submodules"
 	)
 '
 
 test_expect_success 'test submodule set-url' '
-	# add a commit and move the submodule (change the url)
+	# add commits and move the submodules (change the urls)
 	(
 		cd submodule &&
 		echo b >>file &&
 		git add file &&
 		git commit -mb
 	) &&
 	mv submodule newsubmodule &&
 
+	(
+		cd namedsubmodule &&
+		echo 2 >>file &&
+		git add file &&
+		git commit -m2
+	) &&
+	mv namedsubmodule newnamedsubmodule &&
+
 	git -C newsubmodule show >expect &&
+	git -C newnamedsubmodule show >>expect &&
 	(
 		cd super &&
 		test_must_fail git submodule update --remote &&
 		git submodule set-url submodule ../newsubmodule &&
 		test_cmp_config ../newsubmodule -f .gitmodules submodule.submodule.url &&
+		git submodule set-url thepath ../newnamedsubmodule &&
+		test_cmp_config ../newnamedsubmodule -f .gitmodules submodule.thename.url &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.thepath.url &&
 		git submodule update --remote
 	) &&
 	git -C super/submodule show >actual &&
+	git -C super/thepath show >>actual &&
 	test_cmp expect actual
 '
 
-- 
2.42.0


^ permalink raw reply related

* [PATCH 3/6] t7419: Actually test the branch switching
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
  To: git; +Cc: Jan Alexander Steffens (heftig)
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>

The submodule repo the test set up had the 'topic' branch checked out,
meaning the repo's default branch (HEAD) is the 'topic' branch.

The following tests then pretended to switch between the default branch
and the 'topic' branch. This was papered over by continually adding
commits to the 'topic' branch and checking if the submodule gets updated
to this new commit.

Return the submodule repo to the 'main' branch after setup so we can
actually test the switching behavior.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 t/t7419-submodule-set-branch.sh | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 232065504c..5ac16d0eb7 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -11,23 +11,28 @@ as expected.
 
 TEST_PASSES_SANITIZE_LEAK=true
 TEST_NO_CREATE_REPO=1
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
 . ./test-lib.sh
 
 test_expect_success 'setup' '
 	git config --global protocol.file.allow always
 '
 
 test_expect_success 'submodule config cache setup' '
 	mkdir submodule &&
 	(cd submodule &&
 		git init &&
 		echo a >a &&
 		git add . &&
 		git commit -ma &&
 		git checkout -b topic &&
 		echo b >a &&
 		git add . &&
-		git commit -mb
+		git commit -mb &&
+		git checkout main
 	) &&
 	mkdir super &&
 	(cd super &&
@@ -57,41 +62,38 @@ test_expect_success 'test submodule set-branch --branch' '
 '
 
 test_expect_success 'test submodule set-branch --default' '
-	test_commit -C submodule c &&
 	(cd super &&
 		git submodule set-branch --default submodule &&
 		! grep branch .gitmodules &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
-		c
+		a
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch -b' '
-	test_commit -C submodule b &&
 	(cd super &&
 		git submodule set-branch -b topic submodule &&
 		grep "branch = topic" .gitmodules &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		b
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch -d' '
-	test_commit -C submodule d &&
 	(cd super &&
 		git submodule set-branch -d submodule &&
 		! grep branch .gitmodules &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
-		d
+		a
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
-- 
2.42.0


^ permalink raw reply related

* [PATCH 4/6] t7419, t7420: Use test_cmp_config instead of grepping .gitmodules
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
  To: git; +Cc: Jan Alexander Steffens (heftig)
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>

We have a test function to verify config files. Use it as it's more
precise.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 t/t7419-submodule-set-branch.sh | 10 +++++-----
 t/t7420-submodule-set-url.sh    |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 5ac16d0eb7..3cd30865a7 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -44,53 +44,53 @@ test_expect_success 'submodule config cache setup' '
 
 test_expect_success 'ensure submodule branch is unset' '
 	(cd super &&
-		! grep branch .gitmodules
+		test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch
 	)
 '
 
 test_expect_success 'test submodule set-branch --branch' '
 	(cd super &&
 		git submodule set-branch --branch topic submodule &&
-		grep "branch = topic" .gitmodules &&
+		test_cmp_config topic -f .gitmodules submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		b
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch --default' '
 	(cd super &&
 		git submodule set-branch --default submodule &&
-		! grep branch .gitmodules &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		a
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch -b' '
 	(cd super &&
 		git submodule set-branch -b topic submodule &&
-		grep "branch = topic" .gitmodules &&
+		test_cmp_config topic -f .gitmodules submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		b
 		EOF
 		git -C submodule show -s --pretty=%s >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'test submodule set-branch -d' '
 	(cd super &&
 		git submodule set-branch -d submodule &&
-		! grep branch .gitmodules &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch &&
 		git submodule update --remote &&
 		cat <<-\EOF >expect &&
 		a
diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh
index d6bf62b3ac..aa63d806fe 100755
--- a/t/t7420-submodule-set-url.sh
+++ b/t/t7420-submodule-set-url.sh
@@ -49,7 +49,7 @@ test_expect_success 'test submodule set-url' '
 		cd super &&
 		test_must_fail git submodule update --remote &&
 		git submodule set-url submodule ../newsubmodule &&
-		grep -F "url = ../newsubmodule" .gitmodules &&
+		test_cmp_config ../newsubmodule -f .gitmodules submodule.submodule.url &&
 		git submodule update --remote
 	) &&
 	git -C super/submodule show >actual &&
-- 
2.42.0


^ permalink raw reply related

* [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch}
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
  To: git; +Cc: Jan Alexander Steffens (heftig)
In-Reply-To: <0a0a157f88321d25fdb0be771a454b3410a449f3.camel@archlinux.org>

The commands need a path to a submodule but treated it as the name when
modifying the .gitmodules file, leading to confusion when a submodule's
name does not match its path.

Because calling submodule_from_path initializes the submodule cache, we
need to manually trigger a reread before syncing, as the cache is
missing the config change we just made.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 builtin/submodule--helper.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f6871efd95..f376466a5e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2902,19 +2902,26 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
 		N_("git submodule set-url [--quiet] <path> <newurl>"),
 		NULL
 	};
+	const struct submodule *sub;
 
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
 	if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1]))
 		usage_with_options(usage, options);
 
-	config_name = xstrfmt("submodule.%s.url", path);
+	sub = submodule_from_path(the_repository, null_oid(), path);
 
+	if (!sub)
+		die(_("no submodule mapping found in .gitmodules for path '%s'"),
+		    path);
+
+	config_name = xstrfmt("submodule.%s.url", sub->name);
 	config_set_in_gitmodules_file_gently(config_name, newurl);
-	sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0);
+
+	repo_read_gitmodules (the_repository, 0);
+	sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0);
 
 	free(config_name);
-
 	return 0;
 }
 
@@ -2942,19 +2949,26 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 		N_("git submodule set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
 		NULL
 	};
+	const struct submodule *sub;
 
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
 	if (!opt_branch && !opt_default)
 		die(_("--branch or --default required"));
 
 	if (opt_branch && opt_default)
 		die(_("options '%s' and '%s' cannot be used together"), "--branch", "--default");
 
 	if (argc != 1 || !(path = argv[0]))
 		usage_with_options(usage, options);
 
-	config_name = xstrfmt("submodule.%s.branch", path);
+	sub = submodule_from_path(the_repository, null_oid(), path);
+
+	if (!sub)
+		die(_("no submodule mapping found in .gitmodules for path '%s'"),
+		    path);
+
+	config_name = xstrfmt("submodule.%s.branch", sub->name);
 	ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);
 
 	free(config_name);
-- 
2.42.0


^ permalink raw reply related

* [PATCH 5/6] t7419: Test that we correctly handle renamed submodules
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
  To: git; +Cc: Jan Alexander Steffens (heftig)
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>

Add the submodule again with an explicitly different name and path. Test
that calling set-branch modifies the correct .gitmodules entries. Make
sure we don't create a section named after the path instead of the name.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 t/t7419-submodule-set-branch.sh | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 3cd30865a7..a5d1bc5c54 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -38,7 +38,8 @@ test_expect_success 'submodule config cache setup' '
 	(cd super &&
 		git init &&
 		git submodule add ../submodule &&
-		git commit -m "add submodule"
+		git submodule add --name thename ../submodule thepath &&
+		git commit -m "add submodules"
 	)
 '
 
@@ -100,4 +101,31 @@ test_expect_success 'test submodule set-branch -d' '
 	)
 '
 
+test_expect_success 'test submodule set-branch --branch with named submodule' '
+	(cd super &&
+		git submodule set-branch --branch topic thepath &&
+		test_cmp_config topic -f .gitmodules submodule.thename.branch &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.thepath.branch &&
+		git submodule update --remote &&
+		cat <<-\EOF >expect &&
+		b
+		EOF
+		git -C thepath show -s --pretty=%s >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'test submodule set-branch --default with named submodule' '
+	(cd super &&
+		git submodule set-branch --default thepath &&
+		test_cmp_config "" -f .gitmodules --default "" submodule.thename.branch &&
+		git submodule update --remote &&
+		cat <<-\EOF >expect &&
+		a
+		EOF
+		git -C thepath show -s --pretty=%s >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
-- 
2.42.0


^ permalink raw reply related

* [PATCH 2/6] submodule--helper: return error from set-url when modifying failed
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
  To: git; +Cc: Jan Alexander Steffens (heftig)
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>

set-branch will return an error when setting the config fails so I don't
see why set-url shouldn't. Also skip the sync in this case.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 builtin/submodule--helper.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f376466a5e..e2175083a6 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2890,39 +2890,41 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 
 static int module_set_url(int argc, const char **argv, const char *prefix)
 {
-	int quiet = 0;
+	int quiet = 0, ret;
 	const char *newurl;
 	const char *path;
 	char *config_name;
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("suppress output for setting url of a submodule")),
 		OPT_END()
 	};
 	const char *const usage[] = {
 		N_("git submodule set-url [--quiet] <path> <newurl>"),
 		NULL
 	};
 	const struct submodule *sub;
 
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
 	if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1]))
 		usage_with_options(usage, options);
 
 	sub = submodule_from_path(the_repository, null_oid(), path);
 
 	if (!sub)
 		die(_("no submodule mapping found in .gitmodules for path '%s'"),
 		    path);
 
 	config_name = xstrfmt("submodule.%s.url", sub->name);
-	config_set_in_gitmodules_file_gently(config_name, newurl);
+	ret = config_set_in_gitmodules_file_gently(config_name, newurl);
 
-	repo_read_gitmodules (the_repository, 0);
-	sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0);
+	if (!ret) {
+		repo_read_gitmodules(the_repository, 0);
+		sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0);
+	}
 
 	free(config_name);
-	return 0;
+	return !!ret;
 }
 
 static int module_set_branch(int argc, const char **argv, const char *prefix)
-- 
2.42.0


^ permalink raw reply related

* Re: [PATCH 1/1] t2400: avoid using pipes
From: Junio C Hamano @ 2023-10-03 18:42 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: ach.lumap, git, christian.couder
In-Reply-To: <CAPig+cSkZ_brRh_ijFRgz3sP9ou5se9-xeRg=C+cV3c3-v3Wtg@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Oct 3, 2023 at 1:49 PM <ach.lumap@gmail.com> wrote:
>> t2400: avoid using pipes
>
> Pipes themselves are not necessarily problematic, and there are many
> places in the test suite where they are legitimately used. Rather...
> ...
> readers of `git log --oneline` (or other such commands), a better
> subject for the patch might be:
>
>     t2400: avoid losing Git exit code
>
> That minor comment aside (which is probably not worth a reroll), the
> commit message properly explains why this change is desirable and the
> patch itself looks good.

Thanks for writing and reviewing.  Will queue.

^ permalink raw reply

* Re: [PATCH 3/5] git-jump: admit to passing merge mode args to ls-files
From: Eric Sunshine @ 2023-10-03 18:33 UTC (permalink / raw)
  To: Štěpán Němec; +Cc: git
In-Reply-To: <20231003082107.3002173-3-stepnem@smrk.net>

On Tue, Oct 3, 2023 at 4:28 AM Štěpán Němec <stepnem@smrk.net> wrote:
> There's even an example of such usage in the README.
>
> Signed-off-by: Štěpán Němec <stepnem@smrk.net>
> ---
> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
> @@ -9,7 +9,7 @@ The <mode> parameter is one of:
>
>  diff: elements are diff hunks. Arguments are given to diff.
>
> -merge: elements are merge conflicts. Arguments are ignored.
> +merge: elements are merge conflicts. Arguments are given to ls-files -u.

Should "ls-files -u" be formatted with backticks?

    Arguments are passed to `git ls-files -u`.

^ permalink raw reply

* Re: [PATCH 1/5] Fix some typos, grammar or wording issues in the documentation
From: Eric Sunshine @ 2023-10-03 18:30 UTC (permalink / raw)
  To: Štěpán Němec; +Cc: git
In-Reply-To: <20231003082107.3002173-1-stepnem@smrk.net>

On Tue, Oct 3, 2023 at 4:28 AM Štěpán Němec <stepnem@smrk.net> wrote:
> Fix some typos, grammar or wording issues in the documentation

SubmittingPatches suggests: s/Fix/fix

Overall, these changes are welcome improvements. I've left a few minor
comments below which may or may not be actionable.

> Signed-off-by: Štěpán Němec <stepnem@smrk.net>
> ---
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> @@ -393,8 +393,8 @@ mailing list{security-ml}, instead of the public mailing list.
>  Learn to use format-patch and send-email if possible.  These commands
>  are optimized for the workflow of sending patches, avoiding many ways
> -your existing e-mail client that is optimized for "multipart/*" mime
> -type e-mails to corrupt and render your patches unusable.
> +your existing e-mail client (often optimized for "multipart/*" MIME
> +type e-mails) might render your patches unusable.

A subjective alternative would have been to use commas to set off the
newly-parenthesized comment. Not worth a reroll.

> diff --git a/Documentation/git.txt b/Documentation/git.txt
> @@ -96,7 +96,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git config
>  This is useful for cases where you want to pass transitory
> -configuration options to git, but are doing so on OS's where
> +configuration options to git, but are doing so on OSes where

Since you're touching this anyhow, it could probably be made clearer
by simply spelling it out: "operating systems"

>  other processes might be able to read your cmdline
>  (e.g. `/proc/self/cmdline`), but not your environ
>  (e.g. `/proc/self/environ`). That behavior is the default on

I wonder if "cmdline" and "environ" would be better spelled out, as
well, since the examples which immediately follow them give the
necessary context.

    other processes might be able to read your command-line
    (e.g. `/proc/self/cmdline`), but not your environment
    (e.g. `/proc/self/environ`).

But perhaps that's outside the scope of this patch?

> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> @@ -1151,7 +1151,7 @@ will be stored via placeholder `%P`.
>  This attribute controls the length of conflict markers left in
> -the work tree file during a conflicted merge.  Only setting to
> +the work tree file during a conflicted merge.  Only setting
>  the value to a positive integer has any meaningful effect.

Or, more simply:

    Only a positive integer has a meaningful effect.

> diff --git a/contrib/README b/contrib/README
> @@ -24,14 +24,14 @@ lesser degree various foreign SCM interfaces, so you know the
>  I expect that things that start their life in the contrib/ area
> -to graduate out of contrib/ once they mature, either by becoming
> +graduate out of contrib/ once they mature, either by becoming

You probably want to add a comma after "area".

Do we want to correct the formatting of this pathname:

    ...in the `contrib/` area...
    ...out of `contrib/` once...

or is that outside the scope of this patch?

> diff --git a/strbuf.h b/strbuf.h
> @@ -12,9 +12,9 @@
> - * strbuf's are meant to be used with all the usual C string and memory
> + * strbufs are meant to be used with all the usual C string and memory

Alternatively:

    strbuf is meant to be used...

>   * APIs. Given that the length of the buffer is known, it's often better to
> - * use the mem* functions than a str* one (memchr vs. strchr e.g.).
> + * use the mem* functions than a str* one (e.g., memchr vs. strchr).
>   * Though, one has to be careful about the fact that str* functions often
>   * stop on NULs and that strbufs may have embedded NULs.

Similar:

    ... and that a strbuf may have...

> diff --git a/t/README b/t/README
> @@ -262,8 +262,8 @@ The argument for --run, <test-selector>, is a list of description
>  suite to include (or exclude, if negated) in the run.  A range is two
> -numbers separated with a dash and matches a range of tests with both
> -ends been included.  You may omit the first or the second number to
> +numbers separated with a dash and matches an inclusive range of tests
> +to run.  You may omit the first or the second number to

Not the fault of this patch, but "matches" seems an odd choice.
Perhaps "specifies" would feel more natural.

>  The argument to --run is split on commas into separate strings,
> @@ -579,11 +579,10 @@ This test harness library does the following things:
> -Here are some recommented styles when writing test case.

Do you want to fix the spelling error while you're here or is that
done in a later patch?

    s/recommented/recommended/

> - - Keep test title the same line with test helper function itself.
> + - Keep test titles and helper function invocations on the same line.

This would be clearer if it was switched around. Either:

    Keep the test_expect_* function call and test title on the same line.

or, more verbosely:

   Place the test title on the same line as the test_expect_* call
   which precedes it.

^ permalink raw reply

* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Oswald Buddenhagen @ 2023-10-03 18:24 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git List, Jeff King, Junio C Hamano
In-Reply-To: <88cb2db8-e5cb-470a-8060-7a1b898c91f9@web.de>

On Tue, Oct 03, 2023 at 07:54:28PM +0200, René Scharfe wrote:
>C++ is not relevant for Git, but C23 is going to to stop supporting
>binary representations other than two's complement as well.
>
it is relevant insofar as that every platform that comes with a recent 
c++ compiler uses two's complement. this then applies to any language, 
regardless of standard.

>Still I don't feel comfortable overriding compiler warnings for
>something pedestrian as a command line parser.  No idea what other
>assumptions are made in compilers around enums.
>
i'm not sure what you're really worried about. if there was an actual 
problem, we'd have noticed by now. as far as i'm concerned, the question 
is only how to codify the status quo in the most elegant way. putting a 
typecast into a macro certainly qualifies in my book.

regards

^ permalink raw reply

* Re: [PATCH 1/1] t2400: avoid using pipes
From: Eric Sunshine @ 2023-10-03 18:01 UTC (permalink / raw)
  To: ach.lumap; +Cc: git, christian.couder
In-Reply-To: <20231003174853.1732-2-ach.lumap@gmail.com>

On Tue, Oct 3, 2023 at 1:49 PM <ach.lumap@gmail.com> wrote:
> t2400: avoid using pipes

Pipes themselves are not necessarily problematic, and there are many
places in the test suite where they are legitimately used. Rather...

> The exit code of the preceding command in a pipe is disregarded,
> so it's advisable to refrain from relying on it. Instead, by
> saving the output of a Git command to a file, we gain the
> ability to examine the exit codes of both commands separately.

... as you correctly explain here, we don't want to lose the exit code
from the Git command. Thus, if you want to convey more information to
readers of `git log --oneline` (or other such commands), a better
subject for the patch might be:

    t2400: avoid losing Git exit code

That minor comment aside (which is probably not worth a reroll), the
commit message properly explains why this change is desirable and the
patch itself looks good.

> Signed-off-by: achluma <ach.lumap@gmail.com>
> ---
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> @@ -468,7 +468,8 @@ test_expect_success 'put a worktree under rebase' '
>                 cd under-rebase &&
>                 set_fake_editor &&
>                 FAKE_LINES="edit 1" git rebase -i HEAD^ &&
> -               git worktree list | grep "under-rebase.*detached HEAD"
> +               git worktree list >actual &&

Thanks for following the style guideline and omitting whitespace
between the redirection operator and the destination file.

> +               grep "under-rebase.*detached HEAD" actual
>         )
>  '
>
> @@ -509,7 +510,8 @@ test_expect_success 'checkout a branch under bisect' '
>                 git bisect start &&
>                 git bisect bad &&
>                 git bisect good HEAD~2 &&
> -               git worktree list | grep "under-bisect.*detached HEAD" &&
> +               git worktree list >actual &&
> +               grep "under-bisect.*detached HEAD" actual &&
>                 test_must_fail git worktree add new-bisect under-bisect &&
>                 ! test -d new-bisect
>         )

^ permalink raw reply

* Re: [PATCH] parse-options: drop unused parse_opt_ctx_t member
From: René Scharfe @ 2023-10-03 18:00 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Git List
In-Reply-To: <CAPig+cRZ_KzyjWjm-G2qn+t-QA_=CL-tMvTSyZBKrmiHK3RQrg@mail.gmail.com>

Am 03.10.23 um 19:38 schrieb Eric Sunshine:
> On Tue, Oct 3, 2023 at 4:55 AM René Scharfe <l.s.r@web.de> wrote:
>> 5c387428f1 (parse-options: don't emit "ambiguous option" for aliases,
>> 2019-04-29) added "updated_options" to struct parse_opt_ctx_t, but it
>> has never been used.  Remove it.
>> ---
>
> Missing sign-off.

Oops, thanks for catching that.  Technically not necessary, I guess,
since the patch is trivial, but here it is:

Signed-off-by: René Scharfe <l.s.r@web.de>

^ permalink raw reply

* Re: What's cooking in git.git (Oct 2023, #01; Mon, 2)
From: Sergey Organov @ 2023-10-03 17:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq34yr3btn.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> I believe I've addressed this in details in my reply here:
>> <87o7hok8dx.fsf@osv.gnss.ru>, and got no further objections from you
>> since then, so I figure I'd ask to finally let the patch in.
>
> You need to know that no response does not mean no objection.  You
> repeated why the less useful combination is what you want, but that
> does not mean the combination deserves to squat on short-and-sweet
> 'd' and prevent others from coming up with a better use for it.

Yep, but I've asked what's better use for -d than "get me diff"? Do you
really have an idea?

Thanks,
-- Sergey Organov

^ permalink raw reply

* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: René Scharfe @ 2023-10-03 17:54 UTC (permalink / raw)
  To: Oswald Buddenhagen; +Cc: Git List, Jeff King, Junio C Hamano
In-Reply-To: <ZRvhEWHWn4nDynD0@ugly>

Am 03.10.23 um 11:38 schrieb Oswald Buddenhagen:
> On Tue, Oct 03, 2023 at 10:49:12AM +0200, René Scharfe wrote:
>> Am 21.09.23 um 12:40 schrieb Oswald Buddenhagen:
>>> On Wed, Sep 20, 2023 at 10:18:10AM +0200, René Scharfe wrote:
>>>> If we base it on type size then we're making assumptions that
>>>> I find hard to justify.
>>>>
>>> the only one i can think of is signedness. i think this can be
>>> safely ignored as long as we use only small positive integers.
>>
>> I don't fully understand the pointer-sign warning, so I'm not
>> confident enough to silence it.
>>
> in theory, differently signed integers may have completely different
> binary representations. but afaik, that only ever mattered for
> negative numbers. and c++20 actually codifies two's complement, which
> was the de-facto standard for decades already. so in practice it just
> means that we may be assigning a value that is outside the range of
> the actual type. but small positive values are compatible between
> signed and unsiged types.

C++ is not relevant for Git, but C23 is going to to stop supporting
binary representations other than two's complement as well.

Still I don't feel comfortable overriding compiler warnings for
something pedestrian as a command line parser.  No idea what other
assumptions are made in compilers around enums.  I'd rather honor
the warnings and avoid any forcing or trickery if possible.  Or at
least leave that to more capable hands.

René

^ permalink raw reply

* [PATCH 1/1] t2400: avoid using pipes
From: ach.lumap @ 2023-10-03 17:48 UTC (permalink / raw)
  To: git; +Cc: christian.couder, achluma
In-Reply-To: <20231003174853.1732-1-ach.lumap@gmail.com>

From: achluma <ach.lumap@gmail.com>

The exit code of the preceding command in a pipe is disregarded,
so it's advisable to refrain from relying on it. Instead, by
saving the output of a Git command to a file, we gain the
ability to examine the exit codes of both commands separately.

Signed-off-by: achluma <ach.lumap@gmail.com>
---
 t/t2400-worktree-add.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index df4aff7825..7ead05bb98 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -468,7 +468,8 @@ test_expect_success 'put a worktree under rebase' '
 		cd under-rebase &&
 		set_fake_editor &&
 		FAKE_LINES="edit 1" git rebase -i HEAD^ &&
-		git worktree list | grep "under-rebase.*detached HEAD"
+		git worktree list >actual && 
+		grep "under-rebase.*detached HEAD" actual
 	)
 '
 
@@ -509,7 +510,8 @@ test_expect_success 'checkout a branch under bisect' '
 		git bisect start &&
 		git bisect bad &&
 		git bisect good HEAD~2 &&
-		git worktree list | grep "under-bisect.*detached HEAD" &&
+		git worktree list >actual && 
+		grep "under-bisect.*detached HEAD" actual &&
 		test_must_fail git worktree add new-bisect under-bisect &&
 		! test -d new-bisect
 	)
-- 
2.41.0.windows.1


^ permalink raw reply related

* [PATCH 0/1] *** Avoid using Pipes ***
From: ach.lumap @ 2023-10-03 17:48 UTC (permalink / raw)
  To: git; +Cc: christian.couder, achluma

From: achluma <achluma@gmail.com>

*** BLURB HERE ***

achluma (1):
  t2400: avoid using pipes

 t/t2400-worktree-add.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)


base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415
-- 
2.41.0.windows.1


^ permalink raw reply

* Re: [PATCH] parse-options: drop unused parse_opt_ctx_t member
From: Eric Sunshine @ 2023-10-03 17:38 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git List
In-Reply-To: <ebcaa9e1-d306-4c93-adec-3f35d7040531@web.de>

On Tue, Oct 3, 2023 at 4:55 AM René Scharfe <l.s.r@web.de> wrote:
> 5c387428f1 (parse-options: don't emit "ambiguous option" for aliases,
> 2019-04-29) added "updated_options" to struct parse_opt_ctx_t, but it
> has never been used.  Remove it.
> ---

Missing sign-off.

^ permalink raw reply

* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Junio C Hamano @ 2023-10-03 17:15 UTC (permalink / raw)
  To: René Scharfe; +Cc: phillip.wood, Jeff King, Oswald Buddenhagen, Git List
In-Reply-To: <6cb09270-04b9-456e-8d7e-97137e56e9e2@web.de>

René Scharfe <l.s.r@web.de> writes:

> +DEFINE_OPTION_VALUE_TYPE(resume_type, enum resume_type);

These are a bit annoying, but because we need a token that can be ## pasted
to form a valid identifier, we cannot help it.

> diff --git a/parse-options.c b/parse-options.c
> index e8e076c3a6..63a2247128 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -85,7 +85,7 @@ static enum parse_opt_result opt_command_mode_error(
>  		if (that == opt ||
>  		    !(that->flags & PARSE_OPT_CMDMODE) ||
>  		    that->value != opt->value ||
> -		    that->defval != *(int *)opt->value)
> +		    that->defval != opt->get_value(opt->value))
>  			continue;

So, instead of assuming the pointer stuffed in opt->value member can
be dereferenced as inteter pointer, we have the get_value method for
the option and invoke it to grab the value, and compare it with the
default value.

> @@ -122,7 +122,8 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
>  	 * is not a grave error, so let it pass.
>  	 */
>  	if ((opt->flags & PARSE_OPT_CMDMODE) &&
> -	    *(int *)opt->value && *(int *)opt->value != opt->defval)
> +	    opt->get_value(opt->value) &&
> +	    opt->get_value(opt->value) != opt->defval)
>  		return opt_command_mode_error(opt, all_opts, flags);

Likewise.

> @@ -160,6 +161,10 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
>  		*(int *)opt->value = unset ? 0 : opt->defval;
>  		return 0;
>
> +	case OPTION_SET_VALUE:
> +		opt->set_value(opt->value, unset ? 0 : opt->defval);
> +		return 0;

Here we see the previous way in the precontext of this hunk that is
used for OPTION_SET_INT, but in the new type-safe-enum world order,
that uses OPTION_SET_VALUE, the set_value method should know what to
do with the pointer that is in opt->value.

> diff --git a/parse-options.h b/parse-options.h
> index 57a7fe9d91..764e7f7896 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -20,6 +20,7 @@ enum parse_opt_type {
>  	OPTION_BITOP,
>  	OPTION_COUNTUP,
>  	OPTION_SET_INT,
> +	OPTION_SET_VALUE,
>  	/* options with arguments (usually) */
>  	OPTION_STRING,
>  	OPTION_INTEGER,
> @@ -158,8 +159,34 @@ struct option {
>  	parse_opt_ll_cb *ll_callback;
>  	intptr_t extra;
>  	parse_opt_subcommand_fn *subcommand_fn;
> +	intptr_t (*get_value)(void *);
> +	void (*set_value)(void *, intptr_t);
>  };

OK.

> +#define DEFINE_OPTION_VALUE_TYPE(type_name, type) \
> +static inline intptr_t type_name##__get(void *void_ptr) \
> +{ \
> +	type *ptr = void_ptr; \
> +	return (intptr_t)*ptr; \
> +} \
> +static inline void type_name##__set(void *void_ptr, intptr_t value) \
> +{ \
> +	type *ptr = void_ptr; \
> +	*ptr = (type)value; \
> +} \
> +static inline void *type_name##__check(type *ptr) \
> +{ \
> +	return ptr; \
> +} \
> +static inline void *type_name##__check(type *ptr)

Fun.  So a typical pattern is that for "enum foo", the foo__get() is
created from the above template and becomes the .get_value method.

Copying from an earlier hunk, the get_value() method is used like
so:

> -		    that->defval != *(int *)opt->value)
> +		    that->defval != opt->get_value(opt->value))

We pass opt->value (which is void *) to foo__get(), we have a local
variable "enum foo *ptr" and assign it in there, and dereference it.
We used to dereference the pointer as if it were a pointer to an
integer, so the type of foo__get() could be "int", but because we
compare it with the .defval member, which is of type "intptr_t", the
return type of the get_value() method being "intptr_t" would make it
consistent here.  I am not sure why defval need to be "intptr_t", and
for the purpose of this topic it would have been cleaner if it were
"int", but that is a tangent (probably somebody uses it as the default
value for a pointer variable and points it at some default object).

The setter is also reasonable.  An earlier hunk used it like so:

> +		opt->set_value(opt->value, unset ? 0 : opt->defval);

opt->value which is (void *) is assigned to "enum foo *ptr", and
using that pointer, "(enum foo)opt->defval" (or 0) is assinged
there.  Pretty straight-forward.

> +DEFINE_OPTION_VALUE_TYPE(int, int);
> +
> +#define OPTION_VALUE(type_name, v) \
> +	.get_value = type_name##__get, \
> +	.set_value = type_name##__set, \
> +	.value = (1 ? (v) : type_name##__check(v))

This is cute.  foo__check() is declared to take "enum foo *" and
returns it as "void *", but because the condition to the ternary
operator is constant "true", it is discarded.  The only expected
effect is to force the compiler to catch type errors when v is not
of type "enum foo *".

Unless it is "void *", I presume?  Then foo__check() would be happy,
but typically OPTION_VALUE() is used as an implementation detail of
OPT_CMDMODE_T() and you are expected to say something like "&variable"
for "v" above, so it would be OK (because you cannot have a variable
of type "void").

Thanks for a fun read.


^ permalink raw reply

* Re: Is git-p4 maintained?
From: Junio C Hamano @ 2023-10-03 16:42 UTC (permalink / raw)
  To: Yuri; +Cc: Git Mailing List
In-Reply-To: <2f9081b4-e34f-38b3-a557-021c54e4384c@tsoft.com>

Yuri <yuri@rawbw.com> writes:

> Is git-p4 maintained?
> Is there any chance for these problems to be fixed?

The last time anybody touched git-p4 was July last year, but
there is no dedicated area maintainer active even back then, as far
as I know (if not, please raise your hand).

But there may be folks willing to help.  When I saw the patch[*] in
July last year, I was surprised to see it almost immediately got a
review to support it.  Your post may attract attention from others
who are familiar with "git p4" and/or the problem you are describing
and you folks may be able to join forces to improve it.

Thanks.

[References]

* https://lore.kernel.org/git/pull.1285.git.git.1657267260405.gitgitgadget@gmail.com/


^ permalink raw reply

* Re: What's cooking in git.git (Oct 2023, #01; Mon, 2)
From: Junio C Hamano @ 2023-10-03 16:33 UTC (permalink / raw)
  To: Sergey Organov; +Cc: git
In-Reply-To: <871qecgpg1.fsf@osv.gnss.ru>

Sergey Organov <sorganov@gmail.com> writes:

> I believe I've addressed this in details in my reply here:
> <87o7hok8dx.fsf@osv.gnss.ru>, and got no further objections from you
> since then, so I figure I'd ask to finally let the patch in.

You need to know that no response does not mean no objection.  You
repeated why the less useful combination is what you want, but that
does not mean the combination deserves to squat on short-and-sweet
'd' and prevent others from coming up with a better use for it.

^ permalink raw reply

* [PATCH v2] git-status.txt: fix minor asciidoc format issue
From: cousteau via GitGitGadget @ 2023-10-03 16:33 UTC (permalink / raw)
  To: git; +Cc: Javier Mora, cousteau, Javier Mora
In-Reply-To: <pull.1591.git.1696193527923.gitgitgadget@gmail.com>

From: Javier Mora <cousteaulecommandant@gmail.com>

The paragraph below the list of short option combinations
isn't correctly formatted, making the result hard to read.

Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
---
    git-status.txt: minor asciidoc format correction
    
    The paragraph below the list of short option combinations was hard to
    read; turns out it wasn't correctly formatted in asciidoc.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1591%2Fcousteaulecommandant%2Fman-git-status-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1591/cousteaulecommandant/man-git-status-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1591

Range-diff vs v1:

 1:  b3c97ca9e0f ! 1:  811885a275f git-status.txt: fix minor asciidoc format issue
     @@ Commit message
      
       ## Documentation/git-status.txt ##
      @@ Documentation/git-status.txt: U           U    unmerged, both modified
     - ....
       
       Submodules have more state and instead report
     + 
      -		M    the submodule has a different HEAD than
      -		     recorded in the index
      -		m    the submodule has modified content
      -		?    the submodule has untracked files
     -+
      +* 'M' = the submodule has a different HEAD than recorded in the index
      +* 'm' = the submodule has modified content
      +* '?' = the submodule has untracked files
     -+
     + 
       since modified content or untracked files in a submodule cannot be added
       via `git add` in the superproject to prepare a commit.
     - 


 Documentation/git-status.txt | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index b27d127b5e2..48f46eb2047 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -246,10 +246,9 @@ U           U    unmerged, both modified
 
 Submodules have more state and instead report
 
-		M    the submodule has a different HEAD than
-		     recorded in the index
-		m    the submodule has modified content
-		?    the submodule has untracked files
+* 'M' = the submodule has a different HEAD than recorded in the index
+* 'm' = the submodule has modified content
+* '?' = the submodule has untracked files
 
 since modified content or untracked files in a submodule cannot be added
 via `git add` in the superproject to prepare a commit.

base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415
-- 
gitgitgadget

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox