* [PATCH 3/3] reftable/pq_test: comment style fix
From: Junio C Hamano @ 2024-01-29 20:28 UTC (permalink / raw)
To: git
In-Reply-To: <20240129202839.2234084-1-gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
reftable/pq_test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/reftable/pq_test.c b/reftable/pq_test.c
index 011b5c7502..c202eff848 100644
--- a/reftable/pq_test.c
+++ b/reftable/pq_test.c
@@ -60,7 +60,7 @@ static void test_pq(void)
if (last) {
EXPECT(strcmp(last, rec->u.ref.refname) < 0);
}
- // this is names[i], so don't dealloc.
+ /* this is names[i], so don't dealloc. */
last = rec->u.ref.refname;
rec->u.ref.refname = NULL;
reftable_record_release(rec);
--
2.43.0-440-gb50a608ba2
^ permalink raw reply related
* [PATCH 1/3] builtin/worktree: comment style fixes
From: Junio C Hamano @ 2024-01-29 20:28 UTC (permalink / raw)
To: git
In-Reply-To: <20240129202839.2234084-1-gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/worktree.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 6d7da11746..9c76b62b02 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -848,21 +848,21 @@ static int add(int ac, const char **av, const char *prefix)
const char *s = worktree_basename(path, &n);
new_branch = xstrndup(s, n);
} else if (opts.orphan) {
- // No-op
+ ; /* no-op */
} else if (opts.detach) {
- // Check HEAD
+ /* Check HEAD */
if (!strcmp(branch, "HEAD"))
can_use_local_refs(&opts);
} else if (ac < 2 && new_branch) {
- // DWIM: Infer --orphan when repo has no refs.
+ /* DWIM: Infer --orphan when repo has no refs. */
opts.orphan = dwim_orphan(&opts, !!opt_track, 0);
} else if (ac < 2) {
- // DWIM: Guess branch name from path.
+ /* DWIM: Guess branch name from path. */
const char *s = dwim_branch(path, &new_branch);
if (s)
branch = s;
- // DWIM: Infer --orphan when repo has no refs.
+ /* DWIM: Infer --orphan when repo has no refs. */
opts.orphan = (!s) && dwim_orphan(&opts, !!opt_track, 1);
} else if (ac == 2) {
struct object_id oid;
--
2.43.0-440-gb50a608ba2
^ permalink raw reply related
* [PATCH 0/3] Comment style fixes
From: Junio C Hamano @ 2024-01-29 20:28 UTC (permalink / raw)
To: git
Among comments in the traditional /* single liner */ ones, our
reviews seem to have missed a few // comments. While they are not
illegal per-se, they look out of place mixed with the /* ... */
style comments, which is the prevailing style used in this project.
Fix those small number of style violations in our own code; files
with borrowed code that have // comments as the prevalent style are
left untouched.
Junio C Hamano (3):
builtin/worktree: comment style fixes
merge-ort.c: comment style fix
reftable/pq_test: comment style fix
builtin/worktree.c | 10 +++++-----
merge-ort.c | 2 +-
reftable/pq_test.c | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
--
2.43.0-440-gb50a608ba2
^ permalink raw reply
* Re: [PATCH 1/2] Makefile: use order-only prereq for UNIT_TEST_BIN
From: SZEDER Gábor @ 2024-01-29 20:22 UTC (permalink / raw)
To: Jeff King; +Cc: git, Phillip Wood
In-Reply-To: <20240129031816.GA2433899@coredump.intra.peff.net>
On Sun, Jan 28, 2024 at 10:18:16PM -0500, Jeff King wrote:
> We build the UNIT_TEST_BIN directory (t/unit-tests/bin) on the fly with
> "mkdir -p". And so the recipe for UNIT_TEST_PROGS, which put their
> output in that directory, depend on UNIT_TEST_BIN to make sure it's
> there.
>
> But using a normal dependency leads to weird outcomes, because the
> timestamp of the directory is important. For example, try this:
>
> $ make
> [...builds everything...]
>
> [now re-build one unit test]
> $ touch t/unit-tests/t-ctype.c
> $ make
> SUBDIR templates
> CC t/unit-tests/t-ctype.o
> LINK t/unit-tests/bin/t-ctype
>
> So far so good. Now running make again should build nothing. But it
> doesn't!
>
> $ make
> SUBDIR templates
> LINK t/unit-tests/bin/t-basic
> LINK t/unit-tests/bin/t-mem-pool
> LINK t/unit-tests/bin/t-strbuf
>
> Er, what? Let's rebuild again:
>
> $ make
> SUBDIR templates
> LINK t/unit-tests/bin/t-ctype
>
> Weird. And now we ping-pong back and forth forever:
>
> $ make
> SUBDIR templates
> LINK t/unit-tests/bin/t-basic
> LINK t/unit-tests/bin/t-mem-pool
> LINK t/unit-tests/bin/t-strbuf
> $ make
> SUBDIR templates
> LINK t/unit-tests/bin/t-ctype
>
> What happens is that writing t/unit-tests/bin/t-ctype updates the mtime
> of the directory t/unit-tests/bin. And then on the next invocation of
> make, all of those other tests are now older and so get rebuilt. And
> back and forth forever.
>
> We can fix this by using an order-only prereq. This is a GNU-ism that
> tells make to only care that the dependency exists at all, and to ignore
> its mtime. It was designed for exactly this sort of situation (the
> documentation example even uses "mkdir").
>
> We already rely on GNU make, so that's not a problem. This particular
> feature was added in GNU make 3.80, released in October 2002. This is
> obviously quite old by date, but it's also worth thinking about macOS,
> as Apple stopped updating packages that switched to GPLv3 tools. In this
> their dev tools ship GNU make 3.81, which is recent enough.
>
> If it is a problem, there are two alternatives:
>
> - we can just "mkdir -p" in the recipe to build the individual
> binaries. This will mean some redundant "mkdir" calls, but only when
> actually invoking the compiler.
>
> - we could stop making the directory on the fly, and just add it with
> a .gitignore of "*". This would work fine, but might be awkward when
> moving back and forth in history.
A third alternative is to use $(call mkdir_p_parent_template) in the
recipe and get rid of the thus unnecessary UNIT_TEST_BIN dependency
and target. It will only run mkdir when needed, and it's a well
established pattern in our Makefile, so you won't have to spend a
paragraph or two arguing about potential problems with GNU-isms :)
On a related note, 'make clean' doesn't remove this 't/unit-tests/bin'
directory.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I may be overly paranoid about the ".gitignore" strategy. I feel like
> I've been bitten by this in the past by things switching from source to
> build (I think with git-remote-testgit). But that's an actual built
> file. Git would probably be OK with the "bin/" directory coming and
> going as a tracked entity, because the index really only cares about
> the file "bin/.gitignore". Still, this make fix was easy enough.
>
> Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 1a62e48759..958f4cd0bf 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3866,7 +3866,7 @@ fuzz-all: $(FUZZ_PROGRAMS)
> $(UNIT_TEST_BIN):
> @mkdir -p $(UNIT_TEST_BIN)
>
> -$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS $(UNIT_TEST_BIN)
> +$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS | $(UNIT_TEST_BIN)
> $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
> $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
>
> --
> 2.43.0.797.g29b680fc68
>
>
^ permalink raw reply
* Re: what should "git clean -n -f [-d] [-x] <pattern>" do?
From: Kristoffer Haugsbakk @ 2024-01-29 19:40 UTC (permalink / raw)
To: Sergey Organov; +Cc: Elijah Newren, git, Junio C Hamano
In-Reply-To: <87il3enc1i.fsf@osv.gnss.ru>
On Sat, Jan 27, 2024, at 14:25, Sergey Organov wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
I agree with Sergey.
Let’s suppose I’ve never used git-clean(1) (and I almost never use
it). I read the man page to find out what it’s about. Oh, it removes
files that I haven’t tracked. That sounds dangerous. But I see under
`-n, --dry-run` that I can simulate what it would do:
“ Don’t actually remove anything, just show what would be done.
Great, this is what I want. So this seems to mean to run `git clean` and
just tell me what would happen. But now I’ve already read that it
requires `--force` in order to do anything. Which means that I don’t
want to just run:
```
git clean --dry-run
```
Since I presume that would give me the “no `--force` provided”
error. Which means that I want to tack on `--force`:
```
git clean --dry-run --force
```
Now I figure that this will run `git clean --force` but switch real
deletion with printing the filenames.[1]
Junio wrote:
> What I find broken is that giving one 'f' and one 'n' in different
> order, i.e. "-f -n" and "-n -f", does not do what I expect. If you
> are choosing between do-it (f) and do-not-do-it (n), you ought to be
> able to rely on the usual last-one-wins rule. That I find broken.
Now suppose I have noticed that some git(1) commands have these
`--[no-]do-it` options. I know that I can leverage this to override a
previous option. And that is useful when I for example have an alias
with `--do-it` but for this invocation I want `--no-do-it`. I read about
`--force` here but see that there is no `--no-force`. I then assume that
the only things that have to do with `--force` or not is that option and
the `requireForce` configuration variable.
I’ve also seen `--force` in other git(1) commands. And they usually are
about some specific scenario rather than the whole command itself, since
e.g. committing one too many times doesn’t really hurt. But I understand
how `--force` applies to all the useful work that git-clean(1) does
because all the useful work is also destructive work. So this is what I
expect from these options in general:
1. `--force`: require for the subset of actions that are potentially
dangerous or may be unwanted in some way
2. `--dry-run`: simulate the action (specifically print everything that
would happen but don’t do anything to `.git`, to untracked files, or
anything else)
And I expect these two to be orthogonal. Because I might want—if the
option is there—to simulate some `--force` (e.g. `git push --force`)
with a `--dry-run`. As in: what would be printed? I wouldn’t expect
`--force` to override `--dry-run`.
† 1: I’m never this careful in real life. But this is about deleting
files without any (from Git) recovery so I guess some prudence is
required in this case.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Y
From: G G @ 2024-01-29 19:28 UTC (permalink / raw)
To: git
Sent from my iPhone
^ permalink raw reply
* Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string
From: Junio C Hamano @ 2024-01-29 18:58 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <xmqqmssohu69.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> So, after thinking about it a bit more, I do not think I agree with
> the NEEDSWORK comment. I can buy "@", but not an arbitrary revision
> name that happens to point at the same commit as HEAD.
One more thing is it might make sense, if we were to allow more than
the literal string "HEAD", is to include the name of the current
branch (e.g., if "git symbolic-ref HEAD" says "refs/heads/main",
then "main") to the set of tokens that the user may use when they
mean to refer to "HEAD". Unlike "newbranch" they are not currently
on, if they know what branch they are on and they know that is what
HEAD refers to, so the likelihood of them wanting to see the command
behave (i.e. the direction of the patch to be selected and the
messages) the same way may be much higher, I would suspect.
But still, the sudden reversal of the direction of the patches may
bring unexpected confusions to uses. I dunno.
> In other
> words, I may be persuaded to thinking into it is a good idea to add:
>
> static inline int user_means_HEAD(const char *a)
> {
> return !strcmp(a, "HEAD") || !strcmp(a, "@");
> }
>
> and replace "!strcmp(rev, "HEAD")" with "user_means_HEAD(rev)", but
> I would not go any further than that.
>
> Thanks.
^ permalink raw reply
* Re: [PATCH] diff: handle NULL meta-info when spawning external diff
From: Junio C Hamano @ 2024-01-29 18:37 UTC (permalink / raw)
To: Jeff King; +Cc: Wilfred Hughes, git
In-Reply-To: <20240129015708.GA1762343@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> $ git diff --no-index foo bar
>> zsh: segmentation fault (core dumped) git diff --no-index foo bar
>
> Thanks for providing a simple reproduction recipe. There's a pretty
> straight-forward fix below, though it leaves open some question of
> whether there's another bug lurking with --no-index (but either way, I
> think we'd want this simple fix as a first step).
Yup, I agree with you that the "--no-index" mode violates the basic
design that "the other path" and "xfrm_msg" go hand-in-hand. In its
two tree comparison mode "git diff --no-index A/ B/", it should be
able to behave sensibly, but in its two files comparison mode to
compare plain regular files 'foo' and 'bar', there is nothing it can
do reasonably, I am afraid. You could say that the change is
renaming 'foo' to create 'bar', and feed consistent data that is
aligned with that rename to external diff, which might be slightly
more logical than showing a change to 'foo' that has no rename
involved (i.e. omitting "other name"), but neither is satisfying.
> But I'm not sure what fallout we might have from changing that behavior
> now. So this patch takes the less-risky option, and simply teaches
> run_external_diff() to avoid passing xfrm_msg when it's NULL. That makes
> it agnostic to whether "other" and "xfrm_msg" always come as a pair. It
> fixes the segfault now, and if we want to change the --no-index "other"
> behavior on top, it will handle that, too.
Sounds sensible.
Thanks. Will queue.
^ permalink raw reply
* Re: [PATCH 1/1] config: add back code comment
From: Junio C Hamano @ 2024-01-29 18:28 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Kristoffer Haugsbakk, git
In-Reply-To: <ZbeMw5tgY9S6k6y6@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> if (!given_config_source.file)
>> + /*
>> + * It is unknown if HOME/.gitconfig exists, so
>> + * we do not know if we should write to XDG
>> + * location; error out even if XDG_CONFIG_HOME
>> + * is set and points at a sane location.
>> + */
>> die(_("$HOME not set"));
>> given_config_source.scope = CONFIG_SCOPE_GLOBAL;
>> } else if (use_system_config) {
>
> Thanks for adding the comment back in! The patch looks good to me.
Yeah, thanks, both.
^ permalink raw reply
* Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string
From: Junio C Hamano @ 2024-01-29 18:27 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <20240128181202.986753-3-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> Add a new function reveq(), which takes repository struct and two revision
> strings as arguments and returns 0 if the revisions point to the same
> object. Passing a rev which does not point to an object is considered
> undefined behavior as the underlying function memcmp() will be called
> with NULL hash strings.
I didn't dug into the patch or the codepath it touches, but
I wonder if it has something (possibly historical) to do with the
fact that these two behave quite differently:
$ git checkout HEAD
$ git checkout HEAD^0
With the former, you will stay on your current branch, while with
the latter you detach at the commit at the tip of your current
branch. Granted that "git checkout -p" is not about moving HEAD but
checking out some files to the worktree from a given tree-ish, anytime
I see code that does strcmp() with a fixed "HEAD" string, that is
one consideration I'd look for.
> Should the return values of repo_get_oid() be checked in reveq()? As
> reveq() is not a global function and is only used in run_add_p(), the
> validity of revisions is already checked beforehand by builtin/checkout.c
> and builtin/reset.c before the call to run_add_p().
If this were to become a public function (even if it somehow turns
out that it is a bad idea to move away from an explicit comparison
with "HEAD", introducing such a function might be useful---I dunno),
it probably makes sense not to burden its potential callers with too
many assumption. But doesn't the fact that the immediate callers
you are introducing already checked the validity of the revisions
tell us something? Would it result in us not needing this new
helper function at all, if we rearranged the code that already
checks the validity so that the actual object names are collected?
Then it would become the matter of running oideq() directly on these
object names, instead of calling a new helper function that
(re)converts from strings to object names and compares them.
> +// Check if two revisions point to the same object. Passing a rev which does not
> +// point to an object is undefined behavior.
Style:
/*
* Our multi-line comments look
* like this.
*/
> +static inline int reveq(struct repository *r, const char *rev1,
> + const char *rev2)
> +{
> + struct object_id oid_rev1, oid_rev2;
> + repo_get_oid(r, rev1, &oid_rev1);
> + repo_get_oid(r, rev2, &oid_rev2);
> +
> + return !oideq(&oid_rev1, &oid_rev2);
Horribly confusing. If oideq() says "yes, they are the same" by
returning 0, then any helper function derived from it to ansewr "are
X and Y the same?" should return 0 when it wants to say "yes, they
are the same" to help developers keep their sanity.
> +}
> +
> static int parse_range(const char **p,
> unsigned long *offset, unsigned long *count)
> {
> @@ -1730,28 +1743,25 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
> s.mode = &patch_mode_stash;
> else if (mode == ADD_P_RESET) {
> /*
> - * NEEDSWORK: Instead of comparing to the literal "HEAD",
> - * compare the commit objects instead so that other ways of
> - * saying the same thing (such as "@") are also handled
> - * appropriately.
> - *
> - * This applies to the cases below too.
> + * The literal string comparison to HEAD below is kept
> + * to handle unborn HEAD.
> */
So, does this change solve the NEEDSWORK comment? On an unborn
HEAD, this would still not allow you to say "@". Only "HEAD" is
supported.
Not that I necessarily agree with the original "NEEDSWORK" comment
(I think it is perfectly fine for this or any other codepaths not to
take "@" as "HEAD"), but if that desire still stands here, should
the resulting comment still mention it with a NEEDSWORK label?
Besides ...
> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> index 747eb5563e..431f34fa9c 100755
> --- a/t/t2016-checkout-patch.sh
> +++ b/t/t2016-checkout-patch.sh
> @@ -12,6 +12,7 @@ test_expect_success 'setup' '
> git commit -m initial &&
> test_tick &&
> test_commit second dir/foo head &&
> + git branch newbranch &&
> set_and_save_state bar bar_work bar_index &&
> save_head
> '
> +# Note: 'newbranch' points to the same commit as HEAD. And it is technically
> +# allowed to name a branch '@' as of now, however in below test '@'
> +# represents the shortcut for HEAD.
> +for opt in "HEAD" "@" "newbranch"
> +do
> + test_expect_success "git checkout -p $opt with NO staged changes: abort" '
> + set_and_save_state dir/foo work head &&
> + test_write_lines n y n | git checkout -p $opt >output &&
> + verify_saved_state bar &&
> + verify_saved_state dir/foo &&
> + test_grep "Discard" output
> + '
I think this change in behaviour, especially for "newbranch" that
used to use the "_nothead" variants of directions and messages, is
way too confusing. Users may consider "HEAD" and "@" the same and
may want them to behave the same way, but the user, when explicitly
naming "newbranch", means they want to "check contents out of that
OTHER thing named 'newbranch', not the current branch"; it may or
may not happen to be pointing at the same commit as HEAD, but if
the user meant to say "check contents out of the current commit,
(partially) reverting the local changes I have", the user would have
said HEAD. After all, the user may not even be immediately aware
that "newbranch" happens to point at the same commit as HEAD.
So, after thinking about it a bit more, I do not think I agree with
the NEEDSWORK comment. I can buy "@", but not an arbitrary revision
name that happens to point at the same commit as HEAD. In other
words, I may be persuaded to thinking into it is a good idea to add:
static inline int user_means_HEAD(const char *a)
{
return !strcmp(a, "HEAD") || !strcmp(a, "@");
}
and replace "!strcmp(rev, "HEAD")" with "user_means_HEAD(rev)", but
I would not go any further than that.
Thanks.
^ permalink raw reply
* Re: what should "git clean -n -f [-d] [-x] <pattern>" do?
From: Jeff King @ 2024-01-29 18:20 UTC (permalink / raw)
To: Sergey Organov; +Cc: Junio C Hamano, Elijah Newren, git
In-Reply-To: <87jzns7a8a.fsf@osv.gnss.ru>
On Mon, Jan 29, 2024 at 12:35:49PM +0300, Sergey Organov wrote:
> >> I'm still arguing in favor of fixing "-n", and I believe a fix is needed
> >> independently from decision about "-f -f".
> >
> > Even though I do not personally like it, I do not think "which
> > between do-it (f) and do-not-do-it (n) do you want to use?" is
> > broken. It sometimes irritates me to find "git clean" (without "-f"
> > or "-n", and with clean.requireForce not disabled) complain, and I
> > personally think "git clean" when clean.requireForce is in effect
> > and no "-n" or "-f" were given should pretend as if "-n" were given.
>
> As a note, I'd consider to get rid of 'clean.requireForce' anyway, as
> its default value provides safe reasonably behaving environment, and I
> fail to see why anybody would need to set it to 'false'.
Please don't. I set it to "false", because I find the default behavior a
pointless roadblock if you are already aware that "git clean" can be
destructive. Surely I can't be the only one.
-Peff
^ permalink raw reply
* Git in GSoC 2024
From: Kaartic Sivaraam @ 2024-01-29 18:16 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Taylor Blau, Junio C Hamano, Victoria Dye
Hi everyone,
GSoC Org Applications for 2024 are open now [1] and are due before
Tuesday, February 6 at 1800 UTC. It's good to see that contributors have
already started working on microprojects this year :-)
I could help as an Org Admin like previous years. I could volunteer as a
mentor depending on the set of available projects that we're able to
gather for this year.
There are one noticeable change to the program that should be highlighted:
"For 2024 we have the concept of Small ~90 hour projects that are a
standard 8 weeks long (they can extended but it is suggest not
extending beyond 12 weeks for the small projects). We aren't
required to have small projects available but might be worth
considering if we have any small projects that contributors could
work on."
The GSoC contributor application deadline is April 2 - 18:00 UTC, so
(co-)mentors and org admins are already welcome to volunteer. As usual,
we also need project ideas to refresh our idea page from last year
(https://git.github.io/SoC-2023-Ideas/). Feel free to share your
thoughts and discuss.
Do feel free to ask if there's anything that needs to be clarified.
Just like previous year, there will be a GSoC Meetup in Brussels during
FOSDEM weekend on Saturday, February 3rd in the evening. If you are
around, interested and haven't received the link to register directly
from Google, let me know so I can send it to you.
There's also seems to be a GSoC stand at FOSDEM on Saturday or Sunday.
It's said to be located in building H, level 1.
[1]:
https://opensource.googleblog.com/2024/01/google-summer-of-code-2024-mentor-organization-applications-now-open.html
--
Sivaraam
^ permalink raw reply
* Re: [PATCH] pack-bitmap: drop unused `reuse_objects`
From: Jeff King @ 2024-01-29 18:14 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
In-Reply-To: <0bbaf9a3591765161872fb71383263edb0c7ef83.1706328008.git.me@ttaylorr.com>
On Fri, Jan 26, 2024 at 11:00:50PM -0500, Taylor Blau wrote:
> This variable is no longer used for doing verbatim pack-reuse (or
> anywhere within pack-bitmap.c) since d2ea031046 (pack-bitmap: don't rely
> on bitmap_git->reuse_objects, 2019-12-18).
>
> Remove it to avoid an unused struct member.
Good catch. This has been sitting unused for quite some time. I wonder
if there is a way to convince the compiler to tell us about unused
struct fields. I guess that would imply a view of the whole linked
program (though in this case the type is local to a single translation
unit). And would probably produce a horrible number of false positives
for structs defined in system headers.
Possibly static analyzers like coverity could help here, but it does not
seem to find this case (or if it did, I could not find it amidst all of
the false positives).
Anyway, your patch is obviously the right thing to do. :)
-Peff
^ permalink raw reply
* [PATCH v2 1/1] config: add back code comment
From: Kristoffer Haugsbakk @ 2024-01-29 17:57 UTC (permalink / raw)
To: gitster; +Cc: Kristoffer Haugsbakk, git, Patrick Steinhardt
In-Reply-To: <cover.1706550761.git.code@khaugsbakk.name>
c15129b699 (config: factor out global config file retrieval, 2024-01-18)
was a refactor that moved some of the code in this function to
`config.c`. However, in the process I managed to drop this code comment
which explains `$HOME not set`.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
Acked-by: Patrick Steinhardt <ps@pks.im>
---
builtin/config.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/builtin/config.c b/builtin/config.c
index 08fe36d499..b55bfae7d6 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -710,6 +710,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)
if (use_global_config) {
given_config_source.file = git_global_config();
if (!given_config_source.file)
+ /*
+ * It is unknown if HOME/.gitconfig exists, so
+ * we do not know if we should write to XDG
+ * location; error out even if XDG_CONFIG_HOME
+ * is set and points at a sane location.
+ */
die(_("$HOME not set"));
given_config_source.scope = CONFIG_SCOPE_GLOBAL;
} else if (use_system_config) {
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/1] config: add back code comment
From: Kristoffer Haugsbakk @ 2024-01-29 17:57 UTC (permalink / raw)
To: gitster; +Cc: Kristoffer Haugsbakk, git, ps
In-Reply-To: <48d66e94ece3b763acbe933561d82157c02a5f58.1706466321.git.code@khaugsbakk.name>
This is a follow-up to the kh/maintenance-use-xdg-when-it-should
[series] which was merged in 12ee4ed506 (Merge branch
'kh/maintenance-use-xdg-when-it-sho.., 2024-01-26).
I dropped a code comment while iterating on a refactor. It still makes
as much sense in this context as before the refactor (it’s a _refactor_
in the sense of “don’t change code behavior”).
The code comment was moved to `config.c` in patch v1 3/4.[1] But review
feedback said that this comment didn’t fit in this new place and that we
shouldn’t `die()` in `git_global_config`. So in v2 3/4[2] I removed the
comment in `git_global_config`. But I forgot to put the comment back to
its original place, where it still makes as much sense as before my
series.
See the cover letter on the first version for the diff when I squash
this patch into c15129b699 (config: factor out global config file
retrieval, 2024-01-18).
Sorry about the churn.
Cc: ps@pks.im
§ Changes in v2
Add an ack trailer.
This is the (tentative) final version. I read (interpreted)
`SubmittingPatches` as saying that the final version should be sent,
even though it’s just to add an additional trailer. I’m open for
feedback on the submission process of course.
I’ve added it after my signoff since it seems preferred to maintain the
chronology (although in this case either choice seems equally
clear). Also it seemed more common in the recent Git log.
🔗 series: https://lore.kernel.org/git/cover.1697660181.git.code@khaugsbakk.name/
🔗 1: https://lore.kernel.org/git/147c767443c35b3b4a5516bf40557f41bb201078.1697660181.git.code@khaugsbakk.name/
🔗 2: https://lore.kernel.org/git/32e5ec7d866ff8fd26554b325812c6e19cb65126.1705267839.git.code@khaugsbakk.name/
Kristoffer Haugsbakk (1):
config: add back code comment
builtin/config.c | 6 ++++++
1 file changed, 6 insertions(+)
Range-diff against v1:
1: 48d66e94ec ! 1: 24f536d575 config: add back code comment
@@ Commit message
which explains `$HOME not set`.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
+ Acked-by: Patrick Steinhardt <ps@pks.im>
## builtin/config.c ##
@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
--
2.43.0
^ permalink raw reply
* Re: [PATCH v2] merge-tree: accept 3 trees as arguments
From: Junio C Hamano @ 2024-01-29 17:49 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Eric Sunshine, Johannes Schindelin
In-Reply-To: <pull.1647.v2.git.1706474063109.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When specifying a merge base explicitly, there is actually no good
> reason why the inputs need to be commits: that's only needed if the
> merge base has to be deduced from the commit graph.
>
> This commit is best viewed with `--color-moved
> --color-moved-ws=allow-indentation-change`.
Thanks, let's merge it down to 'next'.
^ permalink raw reply
* Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources
From: Jeff King @ 2024-01-29 17:49 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Phillip Wood
In-Reply-To: <ZbeLcrjIYd4d7PaB@tanuki>
On Mon, Jan 29, 2024 at 12:26:42PM +0100, Patrick Steinhardt wrote:
> > -UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
> > +UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
> > +UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))
> > +UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
>
> Wouldn't we have to honor `$X` on Windows systems so that the unit tests
> have the expected ".exe" suffix here?
Hmm, good point. It seems like the answer should obviously be "yes", but
Windows CI seemed to pass all the same (and I checked that it indeed ran
the unit tests). Do we only get the $X suffix for MSVC builds or
something? Looks like maybe cygwin, as well.
I imagine the solution is just:
diff --git a/t/Makefile b/t/Makefile
index c5c6e2ef6b..9b9b30f559 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -43,7 +43,7 @@ TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
-UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))
+UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
but it looks like we might need to include config.mak.uname, as well. It
would be nice to identify a build that actually needs it so I can
confirm that the fix works.
-Peff
^ permalink raw reply related
* Re: [PATCH v2] git-p4: use raw string literals for regular expressions
From: Junio C Hamano @ 2024-01-29 17:25 UTC (permalink / raw)
To: James Touton via GitGitGadget; +Cc: git, James Touton
In-Reply-To: <pull.1639.v2.git.1706312496608.gitgitgadget@gmail.com>
"James Touton via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: James Touton <bekenn@gmail.com>
>
> Fixes several Python diagnostics about invalid escape sequences. The
> diagnostics appear for me in Python 3.12, and may appear in earlier
> versions. The fix is to use raw string literals so that backslashes are
> not interpreted as introducing escape sequences. Raw string literals
> are already in use in this file, so adding more does not impact
> toolchain compatibility.
>
> Signed-off-by: James Touton <bekenn@gmail.com>
> ---
> git-p4: use raw string literals for regular expressions
>
> Changes since v1:
>
> * Updated commit message to include the Python version where the
> diagnostics were observed.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1639%2FBekenn%2Fp4-raw-strings-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1639/Bekenn/p4-raw-strings-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1639
>
> Range-diff vs v1:
>
> 1: 1ea38dc4643 ! 1: 122ff28ffbd git-p4: use raw string literals for regular expressions
> @@ Metadata
> ## Commit message ##
> git-p4: use raw string literals for regular expressions
>
> - Fixes several Python diagnostics about invalid escape sequences.
> + Fixes several Python diagnostics about invalid escape sequences. The
> + diagnostics appear for me in Python 3.12, and may appear in earlier
> + versions. The fix is to use raw string literals so that backslashes are
> + not interpreted as introducing escape sequences. Raw string literals
> + are already in use in this file, so adding more does not impact
> + toolchain compatibility.
>
> Signed-off-by: James Touton <bekenn@gmail.com>
>
Thanks. Let's merge it down to 'next'.
^ permalink raw reply
* Re: [PATCH v3 0/2] index-pack: fsck honor checks
From: Junio C Hamano @ 2024-01-29 17:18 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: John Cai, John Cai via GitGitGadget, Jonathan Tan, git
In-Reply-To: <ZbeI0ksoUQEkbt90@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> I'm always a bit hesitant to add trailers referring to off-list reviews
> to commits. It's impossible for a future reader to discover how that
> trailer came to be by just using the mailing list archive, and expecting
> them to use third-party services to verify them feels wrong to me.
>
> It's part of the reason why I'm pushing more into the direction of
> on-list reviews at GitLab. It makes it a lot more obvious how such a
> Reviewed-by came to be and keeps things self-contained on the mailing
> list. It also grows new contributors who are becoming more familiar with
> how the Git mailing list works. If such a review already happened
> internally due to whatever reason then I think it ought to be fine for
> that reviewer to chime in saying that they have already reviewed the
> patch series and that things look good to them.
Thanks. That would improve clarifying a situation like this one
(eh, actually, once it is done this particular situation wouldn't
need any clarification).
^ permalink raw reply
* Re: [PATCH] reftable: honor core.fsync
From: Junio C Hamano @ 2024-01-29 17:15 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <Zbd0i9nOeWWNQ2EW@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> The topic is currently in `next`, but not yet in `master`, so we might
> still squash it in. Junio, please let me know whether you want to do so
> or whether I shall send this fix-up as a new patch. Thanks!
Any commit in 'next' gets improved only by piling incremental
updates on top with explanation (the idea is: if all of us thought
it has been seen enough eyeballs and yet we later find there was
something we all missed, that is worth a separate explanation---the
primary motivation of the change still was good, but for such and
such reasons we missed this case), unless it turns out that the
approach was fundamentally wrong and such an incremental update
boils down to almost reverting the earlier and replacing with the
newer (in which case, we do revert the earlier and replace it with
the newer, in 'next').
Thanks.
^ permalink raw reply
* Re: How to execute a command on git am/rebase/cherry pick --abort ?
From: Konstantin Kharlamov @ 2024-01-29 14:40 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <4dd609ae2a33f3729291ff26b40632c8eb5decae.camel@yandex.ru>
On Mon, 2024-01-29 at 17:35 +0300, Konstantin Kharlamov wrote:
> On Thu, 2024-01-18 at 14:29 +0100, Patrick Steinhardt wrote:
> > On Thu, Jan 18, 2024 at 03:53:21PM +0300, Konstantin Kharlamov
> > wrote:
> > > (please keep me CC'ed, I'm not subscribed)
> > >
> > > Hello!
> > >
> > > There's a well-known problem of git not fully checking out
> > > changes
> > > while doing e.g. `git checkout` and similar commands when you
> > > have
> > > submodules. So e.g. if HEAD changes a submodule commit and you do
> > > an
> > > interactive rebase to HEAD~2, you may be lucky to find a
> > > submodule
> > > commit change in `git diff` (because if you don't get lucky, you
> > > won't
> > > notice that and commit the change to the unrelated HEAD~2).
> > >
> > > As a workaround I have a `git submodule update` inside `post-
> > > checkout`
> > > hook.
> > >
> > > Now, the problem is I still often finding myself having the wrong
> > > submodule ID, and I tracked down that problem to commands such as
> > > `am/rebase/cherry-pick --abort` also not updating the submodule,
> > > nor
> > > executing `post-checkout`.
> > >
> > > I looked through `man githooks` but couldn't find any way to
> > > execute a
> > > `git submodule update` during these aborts.
> > >
> > > Any ideas how to fix these?
> >
> > Are you aware of the `submodule.recurse` config? If set, it should
> > cause
> > git-checkout(1) and many other commands to recurse into submodules
> > and
> > update them accordingly. This should both make your post-checkout
> > hook
> > obsolete and should also work with git-cherry-pick(1) et al.
>
> It doesn't seem to work ☹ I've set it, and now supposed my top commit
> changes submodule. So I do a `git rebase -i HEAD~2` and "reword" the
> previous commit. After "reword"ing is done, git returns me back to
> HEAD
> commit and when I execute `git diff` I see the submodule ID changed ☹
Well, FTR, I just figured out why that happens, but the option being
broken still stands.
So, turns out what happened is that I have `git submodule update` call
in my `post-checkout` hook, which that `submodule.recurse` option
doesn't account for. Unfortunately, if removing it fixes the situation
mentioned above, however git breaks elsewhere: if I know execute "edit"
on the previous commit and use `git diff`, I see the submodule ID in
the changes.
So… I guess my initial question still stands: is there any hook to fix
the problem of git not updating the submodule upon `git rebase --abort`
and co?
^ permalink raw reply
* Re: How to execute a command on git am/rebase/cherry pick --abort ?
From: Konstantin Kharlamov @ 2024-01-29 14:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <Zaknu0nwBucHVJPP@tanuki>
On Thu, 2024-01-18 at 14:29 +0100, Patrick Steinhardt wrote:
> On Thu, Jan 18, 2024 at 03:53:21PM +0300, Konstantin Kharlamov wrote:
> > (please keep me CC'ed, I'm not subscribed)
> >
> > Hello!
> >
> > There's a well-known problem of git not fully checking out changes
> > while doing e.g. `git checkout` and similar commands when you have
> > submodules. So e.g. if HEAD changes a submodule commit and you do
> > an
> > interactive rebase to HEAD~2, you may be lucky to find a submodule
> > commit change in `git diff` (because if you don't get lucky, you
> > won't
> > notice that and commit the change to the unrelated HEAD~2).
> >
> > As a workaround I have a `git submodule update` inside `post-
> > checkout`
> > hook.
> >
> > Now, the problem is I still often finding myself having the wrong
> > submodule ID, and I tracked down that problem to commands such as
> > `am/rebase/cherry-pick --abort` also not updating the submodule,
> > nor
> > executing `post-checkout`.
> >
> > I looked through `man githooks` but couldn't find any way to
> > execute a
> > `git submodule update` during these aborts.
> >
> > Any ideas how to fix these?
>
> Are you aware of the `submodule.recurse` config? If set, it should
> cause
> git-checkout(1) and many other commands to recurse into submodules
> and
> update them accordingly. This should both make your post-checkout
> hook
> obsolete and should also work with git-cherry-pick(1) et al.
It doesn't seem to work ☹ I've set it, and now supposed my top commit
changes submodule. So I do a `git rebase -i HEAD~2` and "reword" the
previous commit. After "reword"ing is done, git returns me back to HEAD
commit and when I execute `git diff` I see the submodule ID changed ☹
^ permalink raw reply
* [PATCH] merge-ort: turn submodule conflict suggestions into an advice
From: Philippe Blain via GitGitGadget @ 2024-01-29 13:29 UTC (permalink / raw)
To: git; +Cc: Calvin Wan, Philippe Blain, Philippe Blain
From: Philippe Blain <levraiphilippeblain@gmail.com>
Add a new advice type 'submoduleMergeConflict' for the error message
shown when a non-trivial submodule conflict is encountered, which was
added in 4057523a40 (submodule merge: update conflict error message,
2022-08-04). That commit mentions making this message an advice as
possible future work.
Update the tests as the expected message now appears on stderr instead
of stdout.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
merge-ort: turn submodule conflict suggestions into an advice
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1661%2Fphil-blain%2Fmerge-submodule-conflict-advice-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1661/phil-blain/merge-submodule-conflict-advice-v1
Pull-Request: https://github.com/git/git/pull/1661
Documentation/config/advice.txt | 3 +++
advice.c | 1 +
advice.h | 1 +
merge-ort.c | 3 ++-
t/t6437-submodule-merge.sh | 14 +++++++-------
t/t7402-submodule-rebase.sh | 2 +-
6 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 25c09175244..32701b96828 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -128,6 +128,9 @@ advice.*::
submoduleAlternateErrorStrategyDie::
Advice shown when a submodule.alternateErrorStrategy option
configured to "die" causes a fatal error.
+ submoduleMergeConflict::
+ Advice shown when a non-trivial submodule merge conflict is
+ encountered.
submodulesNotUpdated::
Advice shown when a user runs a submodule command that fails
because `git submodule update --init` was not run.
diff --git a/advice.c b/advice.c
index f6e4c2f302e..eee27b5bebc 100644
--- a/advice.c
+++ b/advice.c
@@ -73,6 +73,7 @@ static struct {
[ADVICE_STATUS_U_OPTION] = { "statusUoption", 1 },
[ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated", 1 },
[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
+ [ADVICE_SUBMODULE_MERGE_CONFLICT] = { "submoduleMergeConflict", 1 },
[ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead", 1 },
[ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 },
[ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 },
diff --git a/advice.h b/advice.h
index 9d4f49ae38b..7d0a821f5cb 100644
--- a/advice.h
+++ b/advice.h
@@ -47,6 +47,7 @@ enum advice_type {
ADVICE_STATUS_U_OPTION,
ADVICE_SUBMODULES_NOT_UPDATED,
ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
+ ADVICE_SUBMODULE_MERGE_CONFLICT,
ADVICE_SUGGEST_DETACHING_HEAD,
ADVICE_UPDATE_SPARSE_PATH,
ADVICE_WAITING_FOR_EDITOR,
diff --git a/merge-ort.c b/merge-ort.c
index 77ba7f3020c..59f025db26f 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -18,6 +18,7 @@
#include "merge-ort.h"
#include "alloc.h"
+#include "advice.h"
#include "attr.h"
#include "cache-tree.h"
#include "commit.h"
@@ -4555,7 +4556,7 @@ static void print_submodule_conflict_suggestion(struct string_list *csub) {
" - commit the resulting index in the superproject\n"),
tmp.buf, subs.buf);
- printf("%s", msg.buf);
+ advise_if_enabled(ADVICE_SUBMODULE_MERGE_CONFLICT, "%s", msg.buf);
strbuf_release(&subs);
strbuf_release(&tmp);
diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh
index 70650521b04..7a3f1cb27c1 100755
--- a/t/t6437-submodule-merge.sh
+++ b/t/t6437-submodule-merge.sh
@@ -113,7 +113,7 @@ test_expect_success 'merging should conflict for non fast-forward' '
git checkout -b test-nonforward-a b &&
if test "$GIT_TEST_MERGE_ALGORITHM" = ort
then
- test_must_fail git merge c >actual &&
+ test_must_fail git merge c 2>actual &&
sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" &&
grep "$sub_expect" actual
else
@@ -154,9 +154,9 @@ test_expect_success 'merging should conflict for non fast-forward (resolution ex
git rev-parse --short sub-d > ../expect) &&
if test "$GIT_TEST_MERGE_ALGORITHM" = ort
then
- test_must_fail git merge c >actual &&
+ test_must_fail git merge c >actual 2>sub-actual &&
sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" &&
- grep "$sub_expect" actual
+ grep "$sub_expect" sub-actual
else
test_must_fail git merge c 2> actual
fi &&
@@ -181,9 +181,9 @@ test_expect_success 'merging should fail for ambiguous common parent' '
) &&
if test "$GIT_TEST_MERGE_ALGORITHM" = ort
then
- test_must_fail git merge c >actual &&
+ test_must_fail git merge c >actual 2>sub-actual &&
sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" &&
- grep "$sub_expect" actual
+ grep "$sub_expect" sub-actual
else
test_must_fail git merge c 2> actual
fi &&
@@ -227,7 +227,7 @@ test_expect_success 'merging should fail for changes that are backwards' '
git commit -a -m "f" &&
git checkout -b test-backward e &&
- test_must_fail git merge f >actual &&
+ test_must_fail git merge f 2>actual &&
if test "$GIT_TEST_MERGE_ALGORITHM" = ort
then
sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-d)" &&
@@ -535,7 +535,7 @@ test_expect_success 'merging should fail with no merge base' '
git checkout -b b init &&
git add sub &&
git commit -m "b" &&
- test_must_fail git merge a >actual &&
+ test_must_fail git merge a 2>actual &&
if test "$GIT_TEST_MERGE_ALGORITHM" = ort
then
sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short HEAD^1)" &&
diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
index 2b3c363078b..aa2fdc31d1a 100755
--- a/t/t7402-submodule-rebase.sh
+++ b/t/t7402-submodule-rebase.sh
@@ -116,7 +116,7 @@ test_expect_success 'rebasing submodule that should conflict' '
test_tick &&
git commit -m fourth &&
- test_must_fail git rebase --onto HEAD^^ HEAD^ HEAD^0 >actual_output &&
+ test_must_fail git rebase --onto HEAD^^ HEAD^ HEAD^0 2>actual_output &&
git ls-files -s submodule >actual &&
(
cd submodule &&
base-commit: b50a608ba20348cb3dfc16a696816d51780e3f0f
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 5/5] completion: add an use __git_compute_second_level_config_vars_for_section
From: Philippe Blain via GitGitGadget @ 2024-01-29 13:28 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1660.v2.git.git.1706534881.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
In a previous commit we removed some hardcoded config variable names from
function __git_complete_config_variable_name in the completion script by
introducing a new function,
__git_compute_first_level_config_vars_for_section.
The remaining hardcoded config variables are "second level"
configuration variables, meaning 'branch.<name>.upstream',
'remote.<name>.url', etc. where <name> is a user-defined name.
Making use of the new --config-all-for-completion flag to 'git help'
introduced in the previous commit, add a new function,
__git_compute_second_level_config_vars_for_section. This function takes
as argument a config section name and computes the corresponding
second-level config variables, i.e. those that contain a '<' which
indicates the start of a placeholder. Note that as in
__git_compute_first_level_config_vars_for_section added previsouly, we
use indirect expansion instead of associative arrays to stay compatible
with Bash 3 on which macOS is stuck for licensing reasons.
Use this new function and the variables it defines in
__git_complete_config_variable_name to remove hardcoded config
variables, and add a test to verify the new function. Use a single
'case' for all sections with second-level variables names, since the
code for each of them is now exactly the same.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
contrib/completion/git-completion.bash | 71 ++++++++------------------
t/t9902-completion.sh | 10 ++++
2 files changed, 31 insertions(+), 50 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2934ceb7637..0e8fd63bfdb 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2596,6 +2596,13 @@ __git_compute_config_vars ()
__git_config_vars="$(git help --config-for-completion)"
}
+__git_config_vars_all=
+__git_compute_config_vars_all ()
+{
+ test -n "$__git_config_vars_all" ||
+ __git_config_vars_all="$(git help --config-all-for-completion)"
+}
+
__git_compute_first_level_config_vars_for_section ()
{
section="$1"
@@ -2605,6 +2612,15 @@ __git_compute_first_level_config_vars_for_section ()
printf -v "__git_first_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars" | grep -E "^${section}\.[a-z]" | awk -F. '{print $2}')"
}
+__git_compute_second_level_config_vars_for_section ()
+{
+ section="$1"
+ __git_compute_config_vars_all
+ local this_section="__git_second_level_config_vars_for_section_${section}"
+ test -n "${!this_section}" ||
+ printf -v "__git_second_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars_all" | grep -E "^${section}\.<" | awk -F. '{print $3}')"
+}
+
__git_config_sections=
__git_compute_config_sections ()
{
@@ -2749,10 +2765,13 @@ __git_complete_config_variable_name ()
done
case "$cur_" in
- branch.*.*)
+ branch.*.*|guitool.*.*|difftool.*.*|man.*.*|mergetool.*.*|remote.*.*|submodule.*.*|url.*.*)
local pfx="${cur_%.*}."
cur_="${cur_##*.}"
- __gitcomp "remote pushRemote merge mergeOptions rebase" "$pfx" "$cur_" "$sfx"
+ local section="${pfx%.*.}"
+ __git_compute_second_level_config_vars_for_section "${section}"
+ local this_section="__git_second_level_config_vars_for_section_${section}"
+ __gitcomp "${!this_section}" "$pfx" "$cur_" "$sfx"
return
;;
branch.*)
@@ -2765,33 +2784,6 @@ __git_complete_config_variable_name ()
__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
return
;;
- guitool.*.*)
- local pfx="${cur_%.*}."
- cur_="${cur_##*.}"
- __gitcomp "
- argPrompt cmd confirm needsFile noConsole noRescan
- prompt revPrompt revUnmerged title
- " "$pfx" "$cur_" "$sfx"
- return
- ;;
- difftool.*.*)
- local pfx="${cur_%.*}."
- cur_="${cur_##*.}"
- __gitcomp "cmd path" "$pfx" "$cur_" "$sfx"
- return
- ;;
- man.*.*)
- local pfx="${cur_%.*}."
- cur_="${cur_##*.}"
- __gitcomp "cmd path" "$pfx" "$cur_" "$sfx"
- return
- ;;
- mergetool.*.*)
- local pfx="${cur_%.*}."
- cur_="${cur_##*.}"
- __gitcomp "cmd path trustExitCode" "$pfx" "$cur_" "$sfx"
- return
- ;;
pager.*)
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
@@ -2799,15 +2791,6 @@ __git_complete_config_variable_name ()
__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx:- }"
return
;;
- remote.*.*)
- local pfx="${cur_%.*}."
- cur_="${cur_##*.}"
- __gitcomp "
- url proxy fetch push mirror skipDefaultUpdate
- receivepack uploadpack tagOpt pushurl
- " "$pfx" "$cur_" "$sfx"
- return
- ;;
remote.*)
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
@@ -2818,12 +2801,6 @@ __git_complete_config_variable_name ()
__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
return
;;
- submodule.*.*)
- local pfx="${cur_%.*}."
- cur_="${cur_##*.}"
- __gitcomp "url update branch fetchRecurseSubmodules ignore active" "$pfx" "$cur_" "$sfx"
- return
- ;;
submodule.*)
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
@@ -2834,12 +2811,6 @@ __git_complete_config_variable_name ()
__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
return
;;
- url.*.*)
- local pfx="${cur_%.*}."
- cur_="${cur_##*.}"
- __gitcomp "insteadOf pushInsteadOf" "$pfx" "$cur_" "$sfx"
- return
- ;;
*.*)
__git_compute_config_vars
__gitcomp "$__git_config_vars" "" "$cur_" "$sfx"
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index f28d8f531b7..24ff786b273 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2593,6 +2593,16 @@ test_expect_success 'git config - variable name - __git_compute_first_level_conf
submodule.recurse Z
EOF
'
+test_expect_success 'git config - variable name - __git_compute_second_level_config_vars_for_section' '
+ test_completion "git config branch.main." <<-\EOF
+ branch.main.description Z
+ branch.main.remote Z
+ branch.main.pushRemote Z
+ branch.main.merge Z
+ branch.main.mergeOptions Z
+ branch.main.rebase Z
+ EOF
+'
test_expect_success 'git config - value' '
test_completion "git config color.pager " <<-\EOF
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 4/5] builtin/help: add --config-all-for-completion
From: Philippe Blain via GitGitGadget @ 2024-01-29 13:28 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1660.v2.git.git.1706534881.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
There is currently no machine-friendly way to show _all_ configuration
variables from the command line. 'git help --config' does show them all,
but it also sets up the pager. 'git help --config-for-completion' omits
some variables (those containing wildcards, for example) and 'git help
--config-section-for-completion' shows only top-level section names.
In a following commit we will want to have access to a list of all
configuration variables from the Bash completion script. As such, add a
new mode for the command, HELP_ACTION_CONFIG_ALL_FOR_COMPLETION,
triggered by the new option '--config-all-for-completion'. In this mode,
show all variables, just as HELP_ACTION_CONFIG, but do not set up the
pager.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
builtin/help.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/builtin/help.c b/builtin/help.c
index dc1fbe2b986..dacaeb10bf4 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -50,6 +50,7 @@ static enum help_action {
HELP_ACTION_DEVELOPER_INTERFACES,
HELP_ACTION_CONFIG_FOR_COMPLETION,
HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
+ HELP_ACTION_CONFIG_ALL_FOR_COMPLETION,
} cmd_mode;
static const char *html_path;
@@ -86,6 +87,8 @@ static struct option builtin_help_options[] = {
HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "",
HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),
+ OPT_CMDMODE_F(0, "config-all-for-completion", &cmd_mode, "",
+ HELP_ACTION_CONFIG_ALL_FOR_COMPLETION, PARSE_OPT_HIDDEN),
OPT_END(),
};
@@ -670,6 +673,10 @@ int cmd_help(int argc, const char **argv, const char *prefix)
opt_mode_usage(argc, "--config-for-completion", help_format);
list_config_help(SHOW_CONFIG_VARS);
return 0;
+ case HELP_ACTION_CONFIG_ALL_FOR_COMPLETION:
+ opt_mode_usage(argc, "--config-all-for-completion", help_format);
+ list_config_help(SHOW_CONFIG_HUMAN);
+ return 0;
case HELP_ACTION_USER_INTERFACES:
opt_mode_usage(argc, "--user-interfaces", help_format);
list_user_interfaces_help();
--
gitgitgadget
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox