* Bugreport
From: Frank Schwidom @ 2024-01-19 13:25 UTC (permalink / raw)
To: git
This bug exists in possibly all git versions.
$ git init
$ touch a.txt
$ ln -s a.txt d
$ git add .
$ git commit -m + .
[master (root-commit) f6b4468] +
2 files changed, 1 insertion(+)
create mode 100644 a.txt
create mode 120000 d
$ ls -la
total 12
drwxr-xr-x 3 ox ox 4096 Jan 19 14:10 .
drwxr-xr-x 4 ox ox 4096 Jan 19 14:04 ..
drwxr-xr-x 8 ox ox 4096 Jan 19 14:10 .git
-rw-r--r-- 1 ox ox 0 Jan 19 14:10 a.txt
lrwxrwxrwx 1 ox ox 5 Jan 19 14:10 d -> a.txt
$ rm d
$ mkdir d
$ touch d/b.txt
$ git add .
$ git commit . -m +
error: 'd' does not have a commit checked out
fatal: updating files failed
# I expect that git just replaces the link by the directory. But it makes problems.
# Workaround:
$ rm -rf d
$ git add .
$ git commit -m + .
[master 522e6db] +
1 file changed, 1 deletion(-)
delete mode 120000 d
$ mkdir d
$ touch d/b.txt
$ git add .
$ git commit -m + .
[master 8a125ee] +
1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 d/b.txt
[System Info]
git version:
git version 2.43.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.1.0-8-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.25-1 (2023-04-22) x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.37
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
Thanks in advance,
Frank Schwidom
^ permalink raw reply
* Re: [PATCH 08/12] t1415: move reffiles specific tests to t0600
From: Patrick Steinhardt @ 2024-01-19 13:29 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, John Cai
In-Reply-To: <9d10526369525a0ceee2d75742399130ccf885ce.1705521155.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1729 bytes --]
On Wed, Jan 17, 2024 at 07:52:31PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
>
> Move this test into t0600 with other reffiles specific tests since it
> checks for individua loose refs and thus is specific to the reffiles
> backend.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
> t/t0600-reffiles-backend.sh | 20 ++++++++++++++++++++
> t/t1415-worktree-refs.sh | 11 -----------
> 2 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
> index 0b28a2cc5ea..8526e5cf987 100755
> --- a/t/t0600-reffiles-backend.sh
> +++ b/t/t0600-reffiles-backend.sh
> @@ -502,4 +502,24 @@ test_expect_success 'empty reflog' '
> test_must_be_empty err
> '
>
> +# The 'packed-refs' file is stored directly in .git/. This means it is global
> +# to the repository, and can only contain refs that are shared across all
> +# worktrees.
> +test_expect_success 'refs/worktree must not be packed' '
> + test_commit initial &&
> + test_commit wt1 &&
> + test_commit wt2 &&
> + git worktree add wt1 wt1 &&
> + git worktree add wt2 wt2 &&
> + git checkout initial &&
> + git update-ref refs/worktree/foo HEAD &&
> + git -C wt1 update-ref refs/worktree/foo HEAD &&
> + git -C wt2 update-ref refs/worktree/foo HEAD &&
> + git pack-refs --all &&
> + test_path_is_missing .git/refs/tags/wt1 &&
> + test_path_is_file .git/refs/worktree/foo &&
> + test_path_is_file .git/worktrees/wt1/refs/worktree/foo &&
> + test_path_is_file .git/worktrees/wt2/refs/worktree/foo
> +'
Given that this test exercises git-pack-refs(1), should we move it to
t0601-reffiles-pack-refs.sh instead?
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 04/12] t1404: move reffiles specific tests to t0600
From: Patrick Steinhardt @ 2024-01-19 13:27 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, John Cai
In-Reply-To: <0f6fea6d32d242db772fbee0b4aaec044087f53d.1705521155.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 715 bytes --]
On Wed, Jan 17, 2024 at 07:52:27PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
[snip]
> +test_expect_success 'D/F conflict prevents add long + delete short' '
> + df_test refs/df-al-ds --add-del foo/bar foo
> +'
All of the tests using `df_test ()` pass with the reftable backend, the
only thing that's incompatible is that there is an additional prefix in
the "files" backend's error message. So I'd like to drop moving those
D/F conflict tests so that we can instead make them generic in another
iteration.
All the other tests where we verify how the "files" backend behaves when
there are empty directories in the way do make sense to become backend
specific though.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper
From: Jiang Xin @ 2024-01-19 10:56 UTC (permalink / raw)
To: Linus Arver; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <owly1qaei8hw.fsf@fine.c.googlers.com>
On Fri, Jan 19, 2024 at 6:26 AM Linus Arver <linusa@google.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> >> > Remove the restriction in the "connect_helper()" function and give the
> >> > function "process_connect_service()" the opportunity to establish a
> >> > connection using ".connect" or ".stateless_connect" for protocol v2. So
> >> > we can connect with a stateless-rpc and do something useful. E.g., in a
> >> > later commit, implements remote archive for a repository over HTTP
> >> > protocol.
> >> >
> >> > Helped-by: Junio C Hamano <gitster@pobox.com>
> >> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >> > ---
> >> > transport-helper.c | 2 --
> >> > 1 file changed, 2 deletions(-)
> >> >
> >> > diff --git a/transport-helper.c b/transport-helper.c
> >> > index 49811ef176..2e127d24a5 100644
> >> > --- a/transport-helper.c
> >> > +++ b/transport-helper.c
> >> > @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
> >> >
> >> > /* Get_helper so connect is inited. */
> >> > get_helper(transport);
> >> > - if (!data->connect)
> >> > - die(_("operation not supported by protocol"));
> >>
> >> Should we still terminate early here if both data->connect and
> >> data->stateless_connect are not truthy? It would save a few CPU cycles,
> >> but even better, remain true to the the original intent of the code.
> >> Maybe there was a really good reason to terminate early here that we're
> >> not aware of?
> >>
> >
> > It's not necessary to check data->connect here, because it will
> > terminate if fail to connect by calling the function
> > "process_connect_service()".
>
> In the process_connect_service() we have
>
> if (data->connect) {
> ...
> } else if (data->stateless_connect && ...) {
> ...
> }
>
> strbuf_release(&cmdbuf);
> return ret;
>
> and so if both data->connect and data->stateless_connect are false, that
> function could silently do nothing. IOW that function expects the
> connection type to be guaranteed to be set, so it makes sense to check
> for the correctness of this in the connect_helper().
If both data->connect and data->stateless_connect are false,
process_connect_service() will return 0 instead of making a connection
and returning 1. The return value will be checked in the function
connect_helper() as follows:
if (!process_connect_service(transport, name, exec))
die(_("can't connect to subservice %s"), name);
So I think it's not necessary to make double check in connect_helper().
>
> >> But also, what about the case where both are enabled? Should we print an
> >> error message? (Maybe this concern is outside the scope of this series?)
> >
> > In the function "process_connect_service()", we can see that "connect"
> > has a higher priority than "stateless-connect".
>
> What I mean is, does it make sense for connect_helper() to recognize
> invalid or possibly buggy states? IOW, is having both data->connect and
> data->stateless_connect enabled a bug? If we only ever set one or the
> other (we treat them as mutually exclusive) elsewhere in the codebase,
> and if we are doing the sort of "correctness" check in the
> connect_helper(), then it makes sense to detect that both are set and
> print an error or warning (as a programmer bug).
The best position to address the bug that both data->connect and
data->stateless_connect are enabled is in the function get_helper() as
below:
} else if (!strcmp(capname, "connect")) {
data->connect = 1;
} else if (!strcmp(capname, "stateless-connect")) {
data->stateless_connect = 1;
}
... ...
if (data->connect && data->stateless_connect)
die("cannot have both connect and stateless_connect enabled");
I consider this change to be off-topic and it will not be introduced
in this series.
--
Jiang Xin
^ permalink raw reply
* [PATCH 7/7] Documentation: add "special refs" to the glossary
From: Patrick Steinhardt @ 2024-01-19 10:40 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1705659748.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1416 bytes --]
Add the "special refs" term to our glossary.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/glossary-content.txt | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index f7d98c11e3..d71b199955 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -638,6 +638,20 @@ The most notable example is `HEAD`.
An <<def_object,object>> used to temporarily store the contents of a
<<def_dirty,dirty>> working directory and the index for future reuse.
+[[def_special_ref]]special ref::
+ A ref that has different semantics than normal refs. These refs can be
+ accessed via normal Git commands but may not behave the same as a
+ normal ref in some cases.
++
+The following special refs are known to Git:
+
+ - "`FETCH_HEAD`" is written by linkgit:git-fetch[1] or linkgit:git-pull[1]. It
+ may refer to multiple object IDs. Each object ID is annotated with metadata
+ indicating where it was fetched from and its fetch status.
+
+ - "`MERGE_HEAD`" is written by linkgit:git-merge[1] when resolving merge
+ conflicts. It contains all commit IDs which are being merged.
+
[[def_submodule]]submodule::
A <<def_repository,repository>> that holds the history of a
separate project inside another repository (the latter of
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 6/7] refs: redefine special refs
From: Patrick Steinhardt @ 2024-01-19 10:40 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1705659748.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2494 bytes --]
Now that our list of special refs really only contains refs which have
actually-special semantics, let's redefine what makes a special ref.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 33 +++++++--------------------------
1 file changed, 7 insertions(+), 26 deletions(-)
diff --git a/refs.c b/refs.c
index 047c81b1c1..08a900a047 100644
--- a/refs.c
+++ b/refs.c
@@ -1839,13 +1839,10 @@ static int refs_read_special_head(struct ref_store *ref_store,
static int is_special_ref(const char *refname)
{
/*
- * Special references get written and read directly via the filesystem
- * by the subsystems that create them. Thus, they must not go through
- * the reference backend but must instead be read directly. It is
- * arguable whether this behaviour is sensible, or whether it's simply
- * a leaky abstraction enabled by us only having a single reference
- * backend implementation. But at least for a subset of references it
- * indeed does make sense to treat them specially:
+ * Special references are refs that have different semantics compared
+ * to "normal" refs. These refs can thus not be stored in the ref
+ * backend, but must always be accessed via the filesystem. The
+ * following refs are special:
*
* - FETCH_HEAD may contain multiple object IDs, and each one of them
* carries additional metadata like where it came from.
@@ -1853,25 +1850,9 @@ static int is_special_ref(const char *refname)
* - MERGE_HEAD may contain multiple object IDs when merging multiple
* heads.
*
- * There are some exceptions that you might expect to see on this list
- * but which are handled exclusively via the reference backend:
- *
- * - BISECT_EXPECTED_REV
- *
- * - CHERRY_PICK_HEAD
- *
- * - HEAD
- *
- * - ORIG_HEAD
- *
- * - "rebase-apply/" and "rebase-merge/" contain all of the state for
- * rebases, including some reference-like files. These are
- * exclusively read and written via the filesystem and never go
- * through the refdb.
- *
- * Writing or deleting references must consistently go either through
- * the filesystem (special refs) or through the reference backend
- * (normal ones).
+ * Reading, writing or deleting references must consistently go either
+ * through the filesystem (special refs) or through the reference
+ * backend (normal ones).
*/
static const char * const special_refs[] = {
"FETCH_HEAD",
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 5/7] refs: convert MERGE_AUTOSTASH to become a normal pseudo-ref
From: Patrick Steinhardt @ 2024-01-19 10:40 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1705659748.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6359 bytes --]
Similar to the preceding conversion of the AUTO_MERGE pseudo-ref, let's
convert the MERGE_AUTOSTASH ref to become a normal pseudo-ref as well.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
branch.c | 2 +-
builtin/commit.c | 2 +-
builtin/merge.c | 27 +++++++++++++--------------
path.c | 1 -
path.h | 1 -
refs.c | 1 -
repository.c | 1 -
repository.h | 1 -
8 files changed, 15 insertions(+), 21 deletions(-)
diff --git a/branch.c b/branch.c
index c8bd9519e6..6719a181bd 100644
--- a/branch.c
+++ b/branch.c
@@ -819,7 +819,7 @@ void remove_merge_branch_state(struct repository *r)
unlink(git_path_merge_mode(r));
refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
NULL, REF_NO_DEREF);
- save_autostash(git_path_merge_autostash(r));
+ save_autostash_ref(r, "MERGE_AUTOSTASH");
}
void remove_branch_state(struct repository *r, int verbose)
diff --git a/builtin/commit.c b/builtin/commit.c
index 65196a2827..6d1fa71676 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1877,7 +1877,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
&oid, flags);
}
- apply_autostash(git_path_merge_autostash(the_repository));
+ apply_autostash_ref(the_repository, "MERGE_AUTOSTASH");
cleanup:
strbuf_release(&author_ident);
diff --git a/builtin/merge.c b/builtin/merge.c
index ebbe05033e..8f819781cc 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -476,7 +476,7 @@ static void finish(struct commit *head_commit,
run_hooks_l("post-merge", squash ? "1" : "0", NULL);
if (new_head)
- apply_autostash(git_path_merge_autostash(the_repository));
+ apply_autostash_ref(the_repository, "MERGE_AUTOSTASH");
strbuf_release(&reflog_message);
}
@@ -1315,7 +1315,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (abort_current_merge) {
int nargc = 2;
const char *nargv[] = {"reset", "--merge", NULL};
- struct strbuf stash_oid = STRBUF_INIT;
+ char stash_oid_hex[GIT_MAX_HEXSZ + 1];
+ struct object_id stash_oid = {0};
if (orig_argc != 2)
usage_msg_opt(_("--abort expects no arguments"),
@@ -1324,17 +1325,17 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (!file_exists(git_path_merge_head(the_repository)))
die(_("There is no merge to abort (MERGE_HEAD missing)."));
- if (read_oneliner(&stash_oid, git_path_merge_autostash(the_repository),
- READ_ONELINER_SKIP_IF_EMPTY))
- unlink(git_path_merge_autostash(the_repository));
+ if (!read_ref("MERGE_AUTOSTASH", &stash_oid))
+ delete_ref("", "MERGE_AUTOSTASH", &stash_oid, REF_NO_DEREF);
/* Invoke 'git reset --merge' */
ret = cmd_reset(nargc, nargv, prefix);
- if (stash_oid.len)
- apply_autostash_oid(stash_oid.buf);
+ if (!is_null_oid(&stash_oid)) {
+ oid_to_hex_r(stash_oid_hex, &stash_oid);
+ apply_autostash_oid(stash_oid_hex);
+ }
- strbuf_release(&stash_oid);
goto done;
}
@@ -1563,13 +1564,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
}
if (autostash)
- create_autostash(the_repository,
- git_path_merge_autostash(the_repository));
+ create_autostash_ref(the_repository, "MERGE_AUTOSTASH");
if (checkout_fast_forward(the_repository,
&head_commit->object.oid,
&commit->object.oid,
overwrite_ignore)) {
- apply_autostash(git_path_merge_autostash(the_repository));
+ apply_autostash_ref(the_repository, "MERGE_AUTOSTASH");
ret = 1;
goto done;
}
@@ -1655,8 +1655,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
die_ff_impossible();
if (autostash)
- create_autostash(the_repository,
- git_path_merge_autostash(the_repository));
+ create_autostash_ref(the_repository, "MERGE_AUTOSTASH");
/* We are going to make a new commit. */
git_committer_info(IDENT_STRICT);
@@ -1741,7 +1740,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
else
fprintf(stderr, _("Merge with strategy %s failed.\n"),
use_strategies[0]->name);
- apply_autostash(git_path_merge_autostash(the_repository));
+ apply_autostash_ref(the_repository, "MERGE_AUTOSTASH");
ret = 2;
goto done;
} else if (best_strategy == wt_strategy)
diff --git a/path.c b/path.c
index f881c03171..0fb527918b 100644
--- a/path.c
+++ b/path.c
@@ -1588,6 +1588,5 @@ REPO_GIT_PATH_FUNC(merge_msg, "MERGE_MSG")
REPO_GIT_PATH_FUNC(merge_rr, "MERGE_RR")
REPO_GIT_PATH_FUNC(merge_mode, "MERGE_MODE")
REPO_GIT_PATH_FUNC(merge_head, "MERGE_HEAD")
-REPO_GIT_PATH_FUNC(merge_autostash, "MERGE_AUTOSTASH")
REPO_GIT_PATH_FUNC(fetch_head, "FETCH_HEAD")
REPO_GIT_PATH_FUNC(shallow, "shallow")
diff --git a/path.h b/path.h
index f387410f8c..b3233c51fa 100644
--- a/path.h
+++ b/path.h
@@ -175,7 +175,6 @@ const char *git_path_merge_msg(struct repository *r);
const char *git_path_merge_rr(struct repository *r);
const char *git_path_merge_mode(struct repository *r);
const char *git_path_merge_head(struct repository *r);
-const char *git_path_merge_autostash(struct repository *r);
const char *git_path_fetch_head(struct repository *r);
const char *git_path_shallow(struct repository *r);
diff --git a/refs.c b/refs.c
index 906c7e5f27..047c81b1c1 100644
--- a/refs.c
+++ b/refs.c
@@ -1875,7 +1875,6 @@ static int is_special_ref(const char *refname)
*/
static const char * const special_refs[] = {
"FETCH_HEAD",
- "MERGE_AUTOSTASH",
"MERGE_HEAD",
};
size_t i;
diff --git a/repository.c b/repository.c
index a931e3b1b3..7aacb51b65 100644
--- a/repository.c
+++ b/repository.c
@@ -262,7 +262,6 @@ static void repo_clear_path_cache(struct repo_path_cache *cache)
FREE_AND_NULL(cache->merge_rr);
FREE_AND_NULL(cache->merge_mode);
FREE_AND_NULL(cache->merge_head);
- FREE_AND_NULL(cache->merge_autostash);
FREE_AND_NULL(cache->fetch_head);
FREE_AND_NULL(cache->shallow);
}
diff --git a/repository.h b/repository.h
index 47e7d20b59..7a250a6605 100644
--- a/repository.h
+++ b/repository.h
@@ -67,7 +67,6 @@ struct repo_path_cache {
char *merge_rr;
char *merge_mode;
char *merge_head;
- char *merge_autostash;
char *fetch_head;
char *shallow;
};
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 4/7] sequencer: introduce functions to handle autostashes via refs
From: Patrick Steinhardt @ 2024-01-19 10:40 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1705659748.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4887 bytes --]
We're about to convert the MERGE_AUTOSTASH ref to become non-special,
using the refs API instead of direct filesystem access to both read and
write the ref. The current interfaces to write autostashes is entirely
path-based though, so we need to extend them to also support writes via
the refs API instead.
Ideally, we would be able to fully replace the old set of path-based
interfaces. But the sequencer will continue to write state into
"rebase-merge/autostash". This path is not considered to be a ref at all
and will thus stay is-is for now, which requires us to keep both path-
and refs-based interfaces to handle autostashes.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
sequencer.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++----
sequencer.h | 3 +++
2 files changed, 64 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 47c8d17cbb..91de546b32 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4463,12 +4463,17 @@ static enum todo_command peek_command(struct todo_list *todo_list, int offset)
return -1;
}
-void create_autostash(struct repository *r, const char *path)
+static void create_autostash_internal(struct repository *r,
+ const char *path,
+ const char *refname)
{
struct strbuf buf = STRBUF_INIT;
struct lock_file lock_file = LOCK_INIT;
int fd;
+ if (path && refname)
+ BUG("can only pass path or refname");
+
fd = repo_hold_locked_index(r, &lock_file, 0);
refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
if (0 <= fd)
@@ -4495,10 +4500,16 @@ void create_autostash(struct repository *r, const char *path)
strbuf_reset(&buf);
strbuf_add_unique_abbrev(&buf, &oid, DEFAULT_ABBREV);
- if (safe_create_leading_directories_const(path))
- die(_("Could not create directory for '%s'"),
- path);
- write_file(path, "%s", oid_to_hex(&oid));
+ if (path) {
+ if (safe_create_leading_directories_const(path))
+ die(_("Could not create directory for '%s'"),
+ path);
+ write_file(path, "%s", oid_to_hex(&oid));
+ } else {
+ refs_update_ref(get_main_ref_store(r), "", refname,
+ &oid, null_oid(), 0, UPDATE_REFS_DIE_ON_ERR);
+ }
+
printf(_("Created autostash: %s\n"), buf.buf);
if (reset_head(r, &ropts) < 0)
die(_("could not reset --hard"));
@@ -4509,6 +4520,16 @@ void create_autostash(struct repository *r, const char *path)
strbuf_release(&buf);
}
+void create_autostash(struct repository *r, const char *path)
+{
+ create_autostash_internal(r, path, NULL);
+}
+
+void create_autostash_ref(struct repository *r, const char *refname)
+{
+ create_autostash_internal(r, NULL, refname);
+}
+
static int apply_save_autostash_oid(const char *stash_oid, int attempt_apply)
{
struct child_process child = CHILD_PROCESS_INIT;
@@ -4586,6 +4607,41 @@ int apply_autostash_oid(const char *stash_oid)
return apply_save_autostash_oid(stash_oid, 1);
}
+static int apply_save_autostash_ref(struct repository *r, const char *refname,
+ int attempt_apply)
+{
+ struct object_id stash_oid;
+ char stash_oid_hex[GIT_MAX_HEXSZ + 1];
+ int flag, ret;
+
+ if (!refs_ref_exists(get_main_ref_store(r), refname))
+ return 0;
+
+ if (!refs_resolve_ref_unsafe(get_main_ref_store(r), refname,
+ RESOLVE_REF_READING, &stash_oid, &flag))
+ return -1;
+ if (flag & REF_ISSYMREF)
+ return error(_("autostash reference is a symref"));
+
+ oid_to_hex_r(stash_oid_hex, &stash_oid);
+ ret = apply_save_autostash_oid(stash_oid_hex, attempt_apply);
+
+ refs_delete_ref(get_main_ref_store(r), "", refname,
+ &stash_oid, REF_NO_DEREF);
+
+ return ret;
+}
+
+int save_autostash_ref(struct repository *r, const char *refname)
+{
+ return apply_save_autostash_ref(r, refname, 0);
+}
+
+int apply_autostash_ref(struct repository *r, const char *refname)
+{
+ return apply_save_autostash_ref(r, refname, 1);
+}
+
static int checkout_onto(struct repository *r, struct replay_opts *opts,
const char *onto_name, const struct object_id *onto,
const struct object_id *orig_head)
diff --git a/sequencer.h b/sequencer.h
index 913a0f652d..dcef7bb99c 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -225,9 +225,12 @@ void commit_post_rewrite(struct repository *r,
const struct object_id *new_head);
void create_autostash(struct repository *r, const char *path);
+void create_autostash_ref(struct repository *r, const char *refname);
int save_autostash(const char *path);
+int save_autostash_ref(struct repository *r, const char *refname);
int apply_autostash(const char *path);
int apply_autostash_oid(const char *stash_oid);
+int apply_autostash_ref(struct repository *r, const char *refname);
#define SUMMARY_INITIAL_COMMIT (1 << 0)
#define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/7] refs: convert AUTO_MERGE to become a normal pseudo-ref
From: Patrick Steinhardt @ 2024-01-19 10:40 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1705659748.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7274 bytes --]
In 70c70de616 (refs: complete list of special refs, 2023-12-14) we have
inrtoduced a new `is_special_ref()` function that classifies some refs
as being special. The rule is that special refs are exclusively read and
written via the filesystem directly, whereas normal refs exclucsively go
via the refs API.
The intent of that commit was to record the status quo so that we know
to route reads of such special refs consistently. Eventually, the list
should be reduced to its bare minimum of refs which really are special,
namely FETCH_HEAD and MERGE_HEAD.
Follow up on this promise and convert the AUTO_MERGE ref to become a
normal pseudo-ref by using the refs API to both read and write it
instead of accessing the filesystem directly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
branch.c | 3 ++-
builtin/rebase.c | 2 +-
merge-ort.c | 19 ++++++++++++-------
path.c | 1 -
path.h | 1 -
refs.c | 1 -
repository.c | 1 -
repository.h | 1 -
sequencer.c | 12 ++++++++----
9 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/branch.c b/branch.c
index 534594f7f8..c8bd9519e6 100644
--- a/branch.c
+++ b/branch.c
@@ -817,7 +817,8 @@ void remove_merge_branch_state(struct repository *r)
unlink(git_path_merge_rr(r));
unlink(git_path_merge_msg(r));
unlink(git_path_merge_mode(r));
- unlink(git_path_auto_merge(r));
+ refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
+ NULL, REF_NO_DEREF);
save_autostash(git_path_merge_autostash(r));
}
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 995818c28d..5b086f651a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -515,7 +515,7 @@ static int finish_rebase(struct rebase_options *opts)
int ret = 0;
delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
- unlink(git_path_auto_merge(the_repository));
+ delete_ref(NULL, "AUTO_MERGE", NULL, REF_NO_DEREF);
apply_autostash(state_dir_path("autostash", opts));
/*
* We ignore errors in 'git maintenance run --auto', since the
diff --git a/merge-ort.c b/merge-ort.c
index 77ba7f3020..d72fd04f58 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -38,6 +38,7 @@
#include "path.h"
#include "promisor-remote.h"
#include "read-cache-ll.h"
+#include "refs.h"
#include "revision.h"
#include "sparse-index.h"
#include "strmap.h"
@@ -4659,9 +4660,6 @@ void merge_switch_to_result(struct merge_options *opt,
{
assert(opt->priv == NULL);
if (result->clean >= 0 && update_worktree_and_index) {
- const char *filename;
- FILE *fp;
-
trace2_region_enter("merge", "checkout", opt->repo);
if (checkout(opt, head, result->tree)) {
/* failure to function */
@@ -4687,10 +4685,17 @@ void merge_switch_to_result(struct merge_options *opt,
trace2_region_leave("merge", "record_conflicted", opt->repo);
trace2_region_enter("merge", "write_auto_merge", opt->repo);
- filename = git_path_auto_merge(opt->repo);
- fp = xfopen(filename, "w");
- fprintf(fp, "%s\n", oid_to_hex(&result->tree->object.oid));
- fclose(fp);
+ if (refs_update_ref(get_main_ref_store(opt->repo), "", "AUTO_MERGE",
+ &result->tree->object.oid, NULL, REF_NO_DEREF,
+ UPDATE_REFS_MSG_ON_ERR)) {
+ /* failure to function */
+ opt->priv = NULL;
+ result->clean = -1;
+ merge_finalize(opt, result);
+ trace2_region_leave("merge", "write_auto_merge",
+ opt->repo);
+ return;
+ }
trace2_region_leave("merge", "write_auto_merge", opt->repo);
}
if (display_update_msgs)
diff --git a/path.c b/path.c
index 67e2690efe..f881c03171 100644
--- a/path.c
+++ b/path.c
@@ -1589,6 +1589,5 @@ REPO_GIT_PATH_FUNC(merge_rr, "MERGE_RR")
REPO_GIT_PATH_FUNC(merge_mode, "MERGE_MODE")
REPO_GIT_PATH_FUNC(merge_head, "MERGE_HEAD")
REPO_GIT_PATH_FUNC(merge_autostash, "MERGE_AUTOSTASH")
-REPO_GIT_PATH_FUNC(auto_merge, "AUTO_MERGE")
REPO_GIT_PATH_FUNC(fetch_head, "FETCH_HEAD")
REPO_GIT_PATH_FUNC(shallow, "shallow")
diff --git a/path.h b/path.h
index 639372edd9..f387410f8c 100644
--- a/path.h
+++ b/path.h
@@ -176,7 +176,6 @@ const char *git_path_merge_rr(struct repository *r);
const char *git_path_merge_mode(struct repository *r);
const char *git_path_merge_head(struct repository *r);
const char *git_path_merge_autostash(struct repository *r);
-const char *git_path_auto_merge(struct repository *r);
const char *git_path_fetch_head(struct repository *r);
const char *git_path_shallow(struct repository *r);
diff --git a/refs.c b/refs.c
index 20e8f1ff1f..906c7e5f27 100644
--- a/refs.c
+++ b/refs.c
@@ -1874,7 +1874,6 @@ static int is_special_ref(const char *refname)
* (normal ones).
*/
static const char * const special_refs[] = {
- "AUTO_MERGE",
"FETCH_HEAD",
"MERGE_AUTOSTASH",
"MERGE_HEAD",
diff --git a/repository.c b/repository.c
index d7d24d416a..a931e3b1b3 100644
--- a/repository.c
+++ b/repository.c
@@ -263,7 +263,6 @@ static void repo_clear_path_cache(struct repo_path_cache *cache)
FREE_AND_NULL(cache->merge_mode);
FREE_AND_NULL(cache->merge_head);
FREE_AND_NULL(cache->merge_autostash);
- FREE_AND_NULL(cache->auto_merge);
FREE_AND_NULL(cache->fetch_head);
FREE_AND_NULL(cache->shallow);
}
diff --git a/repository.h b/repository.h
index f5269b3730..47e7d20b59 100644
--- a/repository.h
+++ b/repository.h
@@ -68,7 +68,6 @@ struct repo_path_cache {
char *merge_mode;
char *merge_head;
char *merge_autostash;
- char *auto_merge;
char *fetch_head;
char *shallow;
};
diff --git a/sequencer.c b/sequencer.c
index 6f620f5717..47c8d17cbb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2408,7 +2408,8 @@ static int do_pick_commit(struct repository *r,
refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
NULL, REF_NO_DEREF);
unlink(git_path_merge_msg(r));
- unlink(git_path_auto_merge(r));
+ refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
+ NULL, REF_NO_DEREF);
fprintf(stderr,
_("dropping %s %s -- patch contents already upstream\n"),
oid_to_hex(&commit->object.oid), msg.subject);
@@ -2818,7 +2819,8 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
need_cleanup = 1;
}
- unlink(git_path_auto_merge(r));
+ refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
+ NULL, REF_NO_DEREF);
if (!need_cleanup)
return;
@@ -4766,7 +4768,8 @@ static int pick_commits(struct repository *r,
}
unlink(rebase_path_author_script());
unlink(git_path_merge_head(r));
- unlink(git_path_auto_merge(r));
+ refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
+ NULL, REF_NO_DEREF);
refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD",
NULL, REF_NO_DEREF);
@@ -5123,7 +5126,8 @@ static int commit_staged_changes(struct repository *r,
return error(_("could not commit staged changes."));
unlink(rebase_path_amend());
unlink(git_path_merge_head(r));
- unlink(git_path_auto_merge(r));
+ refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
+ NULL, REF_NO_DEREF);
if (final_fixup) {
unlink(rebase_path_fixup_msg());
unlink(rebase_path_squash_msg());
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/7] sequencer: delete REBASE_HEAD in correct repo when picking commits
From: Patrick Steinhardt @ 2024-01-19 10:40 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1705659748.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]
When picking commits, we delete some state before executing the next
sequencer action on interactive rebases. But while we use the correct
repository to calculate paths to state files that need deletion, we use
the repo-less `delete_ref()` function to delete REBASE_HEAD. Thus, if
the sequencer ran in a different repository than `the_repository`, we
would end up deleting the ref in the wrong repository.
Fix this by using `refs_delete_ref()` instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
sequencer.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index b9cbc290ea..6f620f5717 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4767,7 +4767,8 @@ static int pick_commits(struct repository *r,
unlink(rebase_path_author_script());
unlink(git_path_merge_head(r));
unlink(git_path_auto_merge(r));
- delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
+ refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD",
+ NULL, REF_NO_DEREF);
if (item->command == TODO_BREAK) {
if (!opts->verbose)
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 1/7] sequencer: clean up pseudo refs with REF_NO_DEREF
From: Patrick Steinhardt @ 2024-01-19 10:39 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1705659748.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3061 bytes --]
When cleaning up the state-tracking pseudorefs CHERRY_PICK_HEAD or
REVERT_HEAD we do not set REF_NO_DEREF. In the unlikely case where those
refs are a symref we would thus end up deleting the symref targets, and
not the symrefs themselves.
Harden the code to use REF_NO_DEREF to fix this.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
sequencer.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 3cc88d8a80..b9cbc290ea 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -474,7 +474,7 @@ static void print_advice(struct repository *r, int show_hint,
* of the commit itself so remove CHERRY_PICK_HEAD
*/
refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
- NULL, 0);
+ NULL, REF_NO_DEREF);
return;
}
@@ -1667,7 +1667,7 @@ static int do_commit(struct repository *r,
strbuf_release(&sb);
if (!res) {
refs_delete_ref(get_main_ref_store(r), "",
- "CHERRY_PICK_HEAD", NULL, 0);
+ "CHERRY_PICK_HEAD", NULL, REF_NO_DEREF);
unlink(git_path_merge_msg(r));
if (!is_rebase_i(opts))
print_commit_summary(r, NULL, &oid,
@@ -2406,7 +2406,7 @@ static int do_pick_commit(struct repository *r,
} else if (allow == 2) {
drop_commit = 1;
refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
- NULL, 0);
+ NULL, REF_NO_DEREF);
unlink(git_path_merge_msg(r));
unlink(git_path_auto_merge(r));
fprintf(stderr,
@@ -2802,7 +2802,7 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD")) {
if (!refs_delete_ref(get_main_ref_store(r), "",
- "CHERRY_PICK_HEAD", NULL, 0) &&
+ "CHERRY_PICK_HEAD", NULL, REF_NO_DEREF) &&
verbose)
warning(_("cancelling a cherry picking in progress"));
opts.action = REPLAY_PICK;
@@ -2811,7 +2811,7 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
if (refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD")) {
if (!refs_delete_ref(get_main_ref_store(r), "", "REVERT_HEAD",
- NULL, 0) &&
+ NULL, REF_NO_DEREF) &&
verbose)
warning(_("cancelling a revert in progress"));
opts.action = REPLAY_REVERT;
@@ -4116,7 +4116,7 @@ static int do_merge(struct repository *r,
strbuf_release(&ref_name);
refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
- NULL, 0);
+ NULL, REF_NO_DEREF);
rollback_lock_file(&lock);
ret = run_command(&cmd);
@@ -5108,7 +5108,7 @@ static int commit_staged_changes(struct repository *r,
if (refs_ref_exists(get_main_ref_store(r),
"CHERRY_PICK_HEAD") &&
refs_delete_ref(get_main_ref_store(r), "",
- "CHERRY_PICK_HEAD", NULL, 0))
+ "CHERRY_PICK_HEAD", NULL, REF_NO_DEREF))
return error(_("could not remove CHERRY_PICK_HEAD"));
if (unlink(git_path_merge_msg(r)) && errno != ENOENT)
return error_errno(_("could not remove '%s'"),
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/7] refs: convert special refs to become normal pseudo-refs
From: Patrick Steinhardt @ 2024-01-19 10:39 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]
Hi,
this patch series is a follow-up to the one that introduced
`is_special_ref()` [1]. Back then, we agreed that the list of special
refs ought to shrink over time [2] so that only the actually-special
refs FETCH_HEAD and MERGE_HEAD remain, which is exactly what I'm doing
now.
Patrick
[1]: https://public-inbox.org/git/cover.1701243201.git.ps@pks.im/
[2]: https://public-inbox.org/git/ZXlfeWtDgr1GQFCL@tanuki/
Patrick Steinhardt (7):
sequencer: clean up pseudo refs with REF_NO_DEREF
sequencer: delete REBASE_HEAD in correct repo when picking commits
refs: convert AUTO_MERGE to become a normal pseudo-ref
sequencer: introduce functions to handle autostashes via refs
refs: convert MERGE_AUTOSTASH to become a normal pseudo-ref
refs: redefine special refs
Documentation: add "special refs" to the glossary
Documentation/glossary-content.txt | 14 +++++
branch.c | 5 +-
builtin/commit.c | 2 +-
builtin/merge.c | 27 ++++-----
builtin/rebase.c | 2 +-
merge-ort.c | 19 +++---
path.c | 2 -
path.h | 2 -
refs.c | 35 +++--------
repository.c | 2 -
repository.h | 2 -
sequencer.c | 95 ++++++++++++++++++++++++------
sequencer.h | 3 +
13 files changed, 132 insertions(+), 78 deletions(-)
base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [Help wanted] git-credential-osxkeychain support for attributes password_expiry_utc and oauth_refresh_token
From: M Hickford @ 2024-01-19 8:00 UTC (permalink / raw)
To: Git Mailing List
Hi. Git 2.41 introduced credential attributes password_expiry_utc and
oauth_refresh_token [1]. Credential helper git-credential-osxkeychain
currently silently discards these attributes. Is any keen macOS
developer interested in adding support? This would greatly help
git-credential-oauth users on macOS [2].
Details: the design could follow git-credential-libsecret [3]. There
are already tests in t030-credential-external [4].
The work is small enough it might count as a microproject, though it's
atypical because it's platform specific to macOS.
https://git.github.io/General-Microproject-Information/
Kind regards
-M
[1] https://git-scm.com/docs/git-credential/#IOFMT
[2] https://github.com/hickford/git-credential-oauth/issues/42
[3] https://github.com/git/git/blob/a54a84b333adbecf7bc4483c0e36ed5878cac17b/contrib/credential/libsecret/git-credential-libsecret.c#L157-L177
[4] https://github.com/git/git/blob/a54a84b333adbecf7bc4483c0e36ed5878cac17b/t/lib-credential.sh#L436-L538
^ permalink raw reply
* Re: [PATCH v2 3/4] config: factor out global config file retrieval
From: Patrick Steinhardt @ 2024-01-19 7:59 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: git, stolee, Eric Sunshine, Taylor Blau, Junio C Hamano
In-Reply-To: <7f0864ad-c846-42a6-8ddc-85d6be58a4ee@app.fastmail.com>
[-- Attachment #1: Type: text/plain, Size: 1582 bytes --]
On Fri, Jan 19, 2024 at 08:40:51AM +0100, Kristoffer Haugsbakk wrote:
> On Fri, Jan 19, 2024, at 07:18, Patrick Steinhardt wrote:
> > But second, I think that the new function you introduce here has the
> > same issue as the old function that you refactored in the preceding
> > patch: `git_config_global()` isn't very descriptive, and it is also
> > inconsistent the new `git_config_global_paths()`. I'd propose to name
> > the new function something like `git_config_global_preferred_path()` or
> > `git_config_global_path()`.
>
> The choice of `git_config_global` is mostly motivated by it working the
> same way as `git_config_system`:
>
> ```
> given_config_source.file = git_system_config();
> […]
> given_config_source.file = git_global_config();
> ```
>
> (The extra logic imposed by XDG for “global” is implied by `man git
> config`. I don’t know what the guidelines are for spelling that out or not
> in the internal functions.)
>
> Your suggestion makes sense. But should `git_system_config` be renamed as
> well?
Yeah, you're right that `git_system_config()` is bad in the same way. In
fact I think it's worse here because we have both `git_config_system()`
and `git_system_config()`, which has certainly confused me multiple
times in the past. So I'd be happy to see it renamed, as well, either
now or in a follow-up patch series.
But as I said, I don't think it's a prereq for this patch series to land
and others may have differing opinions. So please, go ahead as you deem
fit (or wait for other opinions).
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] precious-files.txt: new document proposing new precious file type
From: Sebastian Thiel @ 2024-01-19 7:51 UTC (permalink / raw)
To: Elijah Newren
Cc: Junio C Hamano, Elijah Newren via GitGitGadget, git,
Josh Triplett, Phillip Wood
In-Reply-To: <CABPp-BF4Bfr3Hfy7atehHvbQds63+GXO9XPJAW3Mb7dvMcCkDg@mail.gmail.com>
Yes, indeed I was a little confused when making the "git commit..." based examples,
thanks for correcting them.
>
> Reminds me of https://www.emacswiki.org/pics/static/TabsSpacesBoth.png
>
> ;-)
>
😅
> Besides, if for a specific file or filetype, accidental additions are
> more important to protect against than accidental nuking, then can't
> folks achieve that by simply using
>
> # Don't let older git versions add the file
> .env.secret
>
> # For newer git versions, override the above; treat it as precious
> (i.e. don't add AND don't accidentally nuke)
> $.env.secret
>
> In contrast, if protection against accidental nuking is more important
> for certain files, one can use just the second line without the first.
>
I am glad I can pull my initial proposition of 'having both syntaxes' off
the table to side with this version - it's gorgeous.
It's easy to forget that the search-order when matching ignore patterns
is back to front, which makes this 'trick' work.
If the insights gained with the last couple of emails would see their digest
in the user-facing documentation, I think precious files wouldn't only become
usable but would also allow projects to make the their choice during
the transition period during which some users will inevitably access the repository
with a Git that doesn't know about precious files yet.
^ permalink raw reply
* Re: [PATCH v2 3/4] config: factor out global config file retrieval
From: Kristoffer Haugsbakk @ 2024-01-19 7:40 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, stolee, Eric Sunshine, Taylor Blau, Junio C Hamano
In-Reply-To: <ZaoUOPsze7rhtT2M@tanuki>
On Fri, Jan 19, 2024, at 07:18, Patrick Steinhardt wrote:
> But second, I think that the new function you introduce here has the
> same issue as the old function that you refactored in the preceding
> patch: `git_config_global()` isn't very descriptive, and it is also
> inconsistent the new `git_config_global_paths()`. I'd propose to name
> the new function something like `git_config_global_preferred_path()` or
> `git_config_global_path()`.
The choice of `git_config_global` is mostly motivated by it working the
same way as `git_config_system`:
```
given_config_source.file = git_system_config();
[…]
given_config_source.file = git_global_config();
```
(The extra logic imposed by XDG for “global” is implied by `man git
config`. I don’t know what the guidelines are for spelling that out or not
in the internal functions.)
Your suggestion makes sense. But should `git_system_config` be renamed as
well?
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v3 0/5] completion: improvements for git-bisect
From: Patrick Steinhardt @ 2024-01-19 7:05 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git, Junio C Hamano
In-Reply-To: <20240118204323.1113859-1-britton.kerin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 654 bytes --]
On Thu, Jan 18, 2024 at 11:43:18AM -0900, Britton Leo Kerin wrote:
> CC to add: CC: Junio C Hamano <gitster@pobox.com>, Patrick Steinhardt <ps@pks.im>
>
> Relative to v2 this only changes a wrong misleading commit message.
>
> Bring completion for bisect up to date with current features.
Thanks for your patches! I've got a few comments to bring them more in
line with how we're doing things in the Git project, but I agree with
the overall direction that they are going into.
It might be a good idea to also add a few tests in t9902-completion.sh
to ensure that the newly introduced completions work as expected.
Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 5/5] completion: git-bisect view recognized but not completed
From: Patrick Steinhardt @ 2024-01-19 7:05 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git, Junio C Hamano
In-Reply-To: <20240118204323.1113859-6-britton.kerin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2552 bytes --]
On Thu, Jan 18, 2024 at 11:43:23AM -0900, Britton Leo Kerin wrote:
The commit subject is a bit weird in that it does not contain any verb,
it only postulates what happens after the change you do here. How about
"completion: complete options for git-bisect view"? You can then explain
why we only complete options, but not the actual subcommand itself in
the commit message a bit more in depth.
> This allows the git-log options to be completed but avoids completion
> ambiguity between visualize and view.
>
> Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
> ---
> contrib/completion/git-completion.bash | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index ad80df6630..87cf7b2561 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1591,13 +1591,22 @@ _git_bisect ()
> term_good=`__git bisect terms --term-good`
> fi
>
> - local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
> + # We will complete any custom terms, but still always complete the
> + # more usual bad/new/good/old because git bisect gives a good error
> + # message if these are given when not in use and that's better than
> + # silent refusal to complete if the user is confused.
This part doesn't have a lot to do with the changes you're doing in this
commit. So it might've been better to add this explanation in the patch
where you introduced completion for "$term_good" and "$term_bad".
Patrick
> + # We want to recognize 'view' but not complete it, because it overlaps
> + # with 'visualize' too much and is just an alias for it.
> + #
> + local completable_subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
> + local all_subcommands="$completable_subcommands view"
>
> - local subcommand="$(__git_find_on_cmdline "$subcommands")"
> + local subcommand="$(__git_find_on_cmdline "$all_subcommands")"
>
> if [ -z "$subcommand" ]; then
> if [ -f "$__git_repo_path"/BISECT_START ]; then
> - __gitcomp "$subcommands"
> + __gitcomp "$completable_subcommands"
> else
> __gitcomp "replay start"
> fi
> @@ -1615,7 +1624,7 @@ _git_bisect ()
> ;;
> esac
> ;;
> - visualize)
> + visualize|view)
> case "$cur" in
> -*)
> __git_complete_log_opts
> --
> 2.43.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 4/5] completion: custom git-bisect terms
From: Patrick Steinhardt @ 2024-01-19 7:04 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git, Junio C Hamano
In-Reply-To: <20240118204323.1113859-5-britton.kerin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]
On Thu, Jan 18, 2024 at 11:43:22AM -0900, Britton Leo Kerin wrote:
Missing an explanation.
> Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
> ---
> contrib/completion/git-completion.bash | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 63ca8082a4..ad80df6630 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1583,10 +1583,19 @@ _git_bisect ()
> {
> __git_has_doubledash && return
>
> - local subcommands="start bad new good old terms skip reset visualize replay log run help"
> + __git_find_repo_path
> +
> + local term_bad term_good
> + if [ -f "$__git_repo_path"/BISECT_START ]; then
> + term_bad=`__git bisect terms --term-bad`
> + term_good=`__git bisect terms --term-good`
> + fi
We do not use backticks in our codebase. Please use `$(cmd ...)`
instead.
Patrick
> + local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
> +
> local subcommand="$(__git_find_on_cmdline "$subcommands")"
> +
> if [ -z "$subcommand" ]; then
> - __git_find_repo_path
> if [ -f "$__git_repo_path"/BISECT_START ]; then
> __gitcomp "$subcommands"
> else
> @@ -1619,7 +1628,7 @@ _git_bisect ()
> esac
>
> case "$subcommand" in
> - bad|new|good|old|reset|skip|start)
> + bad|new|"$term_bad"|good|old|"$term_good"|reset|skip|start)
> __git_complete_refs
> ;;
> *)
> --
> 2.43.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 3/5] completion: move to maintain define-before-use
From: Patrick Steinhardt @ 2024-01-19 7:04 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git, Junio C Hamano
In-Reply-To: <20240118204323.1113859-4-britton.kerin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 9319 bytes --]
On Thu, Jan 18, 2024 at 11:43:21AM -0900, Britton Leo Kerin wrote:
Same here: please explain what the problem is that this patch is trying
to solve in the commit message.
Also, as far as I can see, this patch is actually a prerequisite for the
preceding patch where we already call `__git_complete_log_opts ()`. So a
better way to structure this would be to introduce and move
`__git_complete_log_opts ()` in a preparatory patch, and only then start
calling the function for "visualize" in a subsequent patch.
Patrick
> Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
> ---
> contrib/completion/git-completion.bash | 269 ++++++++++++-------------
> 1 file changed, 134 insertions(+), 135 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c16aded36c..63ca8082a4 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1445,6 +1445,140 @@ _git_archive ()
> __git_complete_file
> }
>
> +# Options that go well for log, shortlog and gitk
> +__git_log_common_options="
> + --not --all
> + --branches --tags --remotes
> + --first-parent --merges --no-merges
> + --max-count=
> + --max-age= --since= --after=
> + --min-age= --until= --before=
> + --min-parents= --max-parents=
> + --no-min-parents --no-max-parents
> +"
> +# Options that go well for log and gitk (not shortlog)
> +__git_log_gitk_options="
> + --dense --sparse --full-history
> + --simplify-merges --simplify-by-decoration
> + --left-right --notes --no-notes
> +"
> +# Options that go well for log and shortlog (not gitk)
> +__git_log_shortlog_options="
> + --author= --committer= --grep=
> + --all-match --invert-grep
> +"
> +# Options accepted by log and show
> +__git_log_show_options="
> + --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
> +"
> +
> +__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
> +
> +__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
> +__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
> +
> +# Check for only porcelain (i.e. not git-rev-list) option (not argument)
> +# and selected option argument completions for git-log options and if any
> +# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
> +# and will be empty on return if no candidates are found.
> +__git_complete_log_opts ()
> +{
> + [ -z "$COMPREPLY" ] || return 1 # Precondition
> +
> + local merge=""
> + if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
> + merge="--merge"
> + fi
> + case "$prev,$cur" in
> + -L,:*:*)
> + return # fall back to Bash filename completion
> + ;;
> + -L,:*)
> + __git_complete_symbol --cur="${cur#:}" --sfx=":"
> + return
> + ;;
> + -G,*|-S,*)
> + __git_complete_symbol
> + return
> + ;;
> + esac
> + case "$cur" in
> + --pretty=*|--format=*)
> + __gitcomp "$__git_log_pretty_formats $(__git_pretty_aliases)
> + " "" "${cur#*=}"
> + return
> + ;;
> + --date=*)
> + __gitcomp "$__git_log_date_formats" "" "${cur##--date=}"
> + return
> + ;;
> + --decorate=*)
> + __gitcomp "full short no" "" "${cur##--decorate=}"
> + return
> + ;;
> + --diff-algorithm=*)
> + __gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
> + return
> + ;;
> + --submodule=*)
> + __gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
> + return
> + ;;
> + --ws-error-highlight=*)
> + __gitcomp "$__git_ws_error_highlight_opts" "" "${cur##--ws-error-highlight=}"
> + return
> + ;;
> + --no-walk=*)
> + __gitcomp "sorted unsorted" "" "${cur##--no-walk=}"
> + return
> + ;;
> + --diff-merges=*)
> + __gitcomp "$__git_diff_merges_opts" "" "${cur##--diff-merges=}"
> + return
> + ;;
> + --*)
> + __gitcomp "
> + $__git_log_common_options
> + $__git_log_shortlog_options
> + $__git_log_gitk_options
> + $__git_log_show_options
> + --root --topo-order --date-order --reverse
> + --follow --full-diff
> + --abbrev-commit --no-abbrev-commit --abbrev=
> + --relative-date --date=
> + --pretty= --format= --oneline
> + --show-signature
> + --cherry-mark
> + --cherry-pick
> + --graph
> + --decorate --decorate= --no-decorate
> + --walk-reflogs
> + --no-walk --no-walk= --do-walk
> + --parents --children
> + --expand-tabs --expand-tabs= --no-expand-tabs
> + $merge
> + $__git_diff_common_options
> + "
> + return
> + ;;
> + -L:*:*)
> + return # fall back to Bash filename completion
> + ;;
> + -L:*)
> + __git_complete_symbol --cur="${cur#-L:}" --sfx=":"
> + return
> + ;;
> + -G*)
> + __git_complete_symbol --pfx="-G" --cur="${cur#-G}"
> + return
> + ;;
> + -S*)
> + __git_complete_symbol --pfx="-S" --cur="${cur#-S}"
> + return
> + ;;
> + esac
> +}
> +
> _git_bisect ()
> {
> __git_has_doubledash && return
> @@ -2052,141 +2186,6 @@ _git_ls_tree ()
> __git_complete_file
> }
>
> -# Options that go well for log, shortlog and gitk
> -__git_log_common_options="
> - --not --all
> - --branches --tags --remotes
> - --first-parent --merges --no-merges
> - --max-count=
> - --max-age= --since= --after=
> - --min-age= --until= --before=
> - --min-parents= --max-parents=
> - --no-min-parents --no-max-parents
> -"
> -# Options that go well for log and gitk (not shortlog)
> -__git_log_gitk_options="
> - --dense --sparse --full-history
> - --simplify-merges --simplify-by-decoration
> - --left-right --notes --no-notes
> -"
> -# Options that go well for log and shortlog (not gitk)
> -__git_log_shortlog_options="
> - --author= --committer= --grep=
> - --all-match --invert-grep
> -"
> -# Options accepted by log and show
> -__git_log_show_options="
> - --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
> -"
> -
> -__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
> -
> -__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
> -__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
> -
> -
> -# Check for only porcelain (i.e. not git-rev-list) option (not argument)
> -# and selected option argument completions for git-log options and if any
> -# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
> -# and will be empty on return if no candidates are found.
> -__git_complete_log_opts ()
> -{
> - [ -z "$COMPREPLY" ] || return 1 # Precondition
> -
> - local merge=""
> - if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
> - merge="--merge"
> - fi
> - case "$prev,$cur" in
> - -L,:*:*)
> - return # fall back to Bash filename completion
> - ;;
> - -L,:*)
> - __git_complete_symbol --cur="${cur#:}" --sfx=":"
> - return
> - ;;
> - -G,*|-S,*)
> - __git_complete_symbol
> - return
> - ;;
> - esac
> - case "$cur" in
> - --pretty=*|--format=*)
> - __gitcomp "$__git_log_pretty_formats $(__git_pretty_aliases)
> - " "" "${cur#*=}"
> - return
> - ;;
> - --date=*)
> - __gitcomp "$__git_log_date_formats" "" "${cur##--date=}"
> - return
> - ;;
> - --decorate=*)
> - __gitcomp "full short no" "" "${cur##--decorate=}"
> - return
> - ;;
> - --diff-algorithm=*)
> - __gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
> - return
> - ;;
> - --submodule=*)
> - __gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
> - return
> - ;;
> - --ws-error-highlight=*)
> - __gitcomp "$__git_ws_error_highlight_opts" "" "${cur##--ws-error-highlight=}"
> - return
> - ;;
> - --no-walk=*)
> - __gitcomp "sorted unsorted" "" "${cur##--no-walk=}"
> - return
> - ;;
> - --diff-merges=*)
> - __gitcomp "$__git_diff_merges_opts" "" "${cur##--diff-merges=}"
> - return
> - ;;
> - --*)
> - __gitcomp "
> - $__git_log_common_options
> - $__git_log_shortlog_options
> - $__git_log_gitk_options
> - $__git_log_show_options
> - --root --topo-order --date-order --reverse
> - --follow --full-diff
> - --abbrev-commit --no-abbrev-commit --abbrev=
> - --relative-date --date=
> - --pretty= --format= --oneline
> - --show-signature
> - --cherry-mark
> - --cherry-pick
> - --graph
> - --decorate --decorate= --no-decorate
> - --walk-reflogs
> - --no-walk --no-walk= --do-walk
> - --parents --children
> - --expand-tabs --expand-tabs= --no-expand-tabs
> - $merge
> - $__git_diff_common_options
> - "
> - return
> - ;;
> - -L:*:*)
> - return # fall back to Bash filename completion
> - ;;
> - -L:*)
> - __git_complete_symbol --cur="${cur#-L:}" --sfx=":"
> - return
> - ;;
> - -G*)
> - __git_complete_symbol --pfx="-G" --cur="${cur#-G}"
> - return
> - ;;
> - -S*)
> - __git_complete_symbol --pfx="-S" --cur="${cur#-S}"
> - return
> - ;;
> - esac
> -}
> -
> _git_log ()
> {
> __git_has_doubledash && return
> --
> 2.43.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 2/5] completion: git-log opts to bisect visualize
From: Patrick Steinhardt @ 2024-01-19 7:04 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git, Junio C Hamano
In-Reply-To: <20240118204323.1113859-3-britton.kerin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3728 bytes --]
On Thu, Jan 18, 2024 at 11:43:20AM -0900, Britton Leo Kerin wrote:
Proposal for the commit subject: "completion: complete log opts for
git-bisect visualize".
> To do this the majority of _git_log has been factored out into the new
> __git_complete_log_opts.
We typically do not continue the commit message as if the commit subject
was the first line of the message. An introduction like the following
would help to set the stage:
Arguments passed to the "visualize" subcommand of git-bisect(1) get
forwarded to git-log(1). It thus supports the same options as
git-log(1) would, but our Bash completion script does not know to
handle this.
> This is needed because the visualize command
> accepts git-log options but not rev arguments (they are fixed to the
> commits under bisection).
>
> __git_complete_log_opts has a precondition that COMPREPLY be empty. In
> a completion context it doesn't seem advisable to implement
> preconditions as noisy or hard failures, so instead it becomes a no-op
> on violation. This should be detectable and quick to debug for devels,
> without ever aggravating a user (besides completion failure).
>
> Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
> ---
> contrib/completion/git-completion.bash | 30 +++++++++++++++++++++++---
> 1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 15d22ff7d9..c16aded36c 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1472,6 +1472,16 @@ _git_bisect ()
> ;;
> esac
> ;;
> + visualize)
> + case "$cur" in
> + -*)
> + __git_complete_log_opts
> + return
> + ;;
> + *)
> + ;;
> + esac
> + ;;
Is this switch even needed? Can't we call `__git_complete_log_opts`
directly?
> esac
>
> case "$subcommand" in
> @@ -2074,10 +2084,14 @@ __git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-c
> __git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
> __git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
>
> -_git_log ()
> +
> +# Check for only porcelain (i.e. not git-rev-list) option (not argument)
> +# and selected option argument completions for git-log options and if any
> +# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
> +# and will be empty on return if no candidates are found.
Why do we need to enforce that COMPREPLY is empty? None of the other
`__git_complete_*` helpers do this, so I think it's fair to expect that
the variable woulld get clobbered when calling the new function. Thus, I
don't think there's a need for this precondition.
> +__git_complete_log_opts ()
> {
> - __git_has_doubledash && return
> - __git_find_repo_path
> + [ -z "$COMPREPLY" ] || return 1 # Precondition
>
> local merge=""
> if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
> @@ -2171,6 +2185,16 @@ _git_log ()
> return
> ;;
> esac
> +}
> +
> +_git_log ()
> +{
> + __git_has_doubledash && return
> + __git_find_repo_path
I was about to say that it would make more sense to call
`__git_find_repo_path` in `__git_complete_log_opts` so that all prereqs
are fulfilled whenever the latter is called. But `__git_complete_relist`
doesn't know to find the repo path in all cases, so that wouldn't quite
work alright.
Patrick
> + __git_complete_log_opts
> + [ -z "$COMPREPLY" ] || return
> +
> __git_complete_revlist
> }
>
> --
> 2.43.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/5] completion: complete new old actions, start opts
From: Patrick Steinhardt @ 2024-01-19 7:04 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git, Junio C Hamano
In-Reply-To: <20240118204323.1113859-2-britton.kerin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2198 bytes --]
On Thu, Jan 18, 2024 at 11:43:19AM -0900, Britton Leo Kerin wrote:
I feel like the commit message can be improved. First, you don't mention
that you're adding completion for git-bisect(1), which one needs to
infer from the patch. Second, the message is missing an explanation what
is wrong about the current code and how you're fixing it.
> Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
> ---
> contrib/completion/git-completion.bash | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 185b47d802..15d22ff7d9 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1449,7 +1449,7 @@ _git_bisect ()
> {
> __git_has_doubledash && return
>
> - local subcommands="start bad good skip reset visualize replay log run"
> + local subcommands="start bad new good old terms skip reset visualize replay log run help"
I'd propose to move the addition of subcommands into one or more
additional commits. "bad", "old" and "help" all already work perfectly
fine, so these can easily go into a single commit. But I think that it
would make sense to introduce "terms" in a standalone commit and then
extend the below case statement to handle its argumens "--term-good" and
"--term-bad".
> local subcommand="$(__git_find_on_cmdline "$subcommands")"
> if [ -z "$subcommand" ]; then
> __git_find_repo_path
> @@ -1462,7 +1462,20 @@ _git_bisect ()
> fi
>
> case "$subcommand" in
> - bad|good|reset|skip|start)
> + start)
> + case "$cur" in
> + --*)
> + __gitcomp "--term-new --term-bad --term-old --term-good --first-parent --no-checkout"
> + return
> + ;;
> + *)
> + ;;
If you also called `__git_complete_refs here` then you could merge the
two case statements for "$subcommand". It's a bit of duplicate code, but
I think the end result would be easier to read.
Patrick
> + esac
> + ;;
> + esac
> +
> + case "$subcommand" in
> + bad|new|good|old|reset|skip|start)
> __git_complete_refs
> ;;
> *)
> --
> 2.43.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] diffcore-delta: avoid ignoring final 'line' of file
From: Junio C Hamano @ 2024-01-19 6:27 UTC (permalink / raw)
To: Elijah Newren; +Cc: Elijah Newren via GitGitGadget, git
In-Reply-To: <CABPp-BHybPEg_+649fL6QrKjRQcJXxbYMFiQn0KxAgbr2Nz0Gg@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> But perhaps I can put it another way: You can't just look at the
> output of `diff --name-only` and say a rename was involved -- unless
> you know the test setup and the previous operations.
That is true. But that is exactly what a test is about. You have
this and that file, and you do this operation, now what should
happen? Does the observation match the expectation? That is what
our tests are done.
And your argument should not have to rely on a bug in "git mv".
After all, you should be able to do the same with "mv A B && git add
B && git add -u" (or "git rm -f A") and you won't be affected by
such a bug.
> If you still like `diff --name-only` better anyway, that's fine and
> I'll switch it. I'm just stating why it seems suboptimal to me.
I'd prefer to omit "sed" involved, but I'd even more prefer not
waste more time on the test. As long as we can make a robust test
(which an extra process running sed would certainly give us), I'd be
fine.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 3/4] config: factor out global config file retrieval
From: Patrick Steinhardt @ 2024-01-19 6:18 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, stolee, Eric Sunshine, Taylor Blau
In-Reply-To: <32e5ec7d866ff8fd26554b325812c6e19cb65126.1705267839.git.code@khaugsbakk.name>
[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]
On Sun, Jan 14, 2024 at 10:43:18PM +0100, Kristoffer Haugsbakk wrote:
> Factor out code that retrieves the global config file so that we can use
> it in `gc.c` as well.
>
> Use the old name from the previous commit since this function acts
> functionally the same as `git_system_config` but for “global”.
I was briefly wondering whether we also want to give this new function a
more descriptive name. For one, calling it `git_system_config()` which
we have just removed in the preceding set may easily lead to confusion
for any in-flight patch series because the parameters now got dropped
(or at least it looks like that).
But second, I think that the new function you introduce here has the
same issue as the old function that you refactored in the preceding
patch: `git_config_global()` isn't very descriptive, and it is also
inconsistent the new `git_config_global_paths()`. I'd propose to name
the new function something like `git_config_global_preferred_path()` or
`git_config_global_path()`.
Sorry for not mentioning this in my first review round. Also, it's only
a minor concern, nothing that needs to block this series if either you
or others disagree with my opinion.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: brianmlyles @ 2024-01-19 5:59 UTC (permalink / raw)
To: git; +Cc: me, newren, Brian Lyles
In-Reply-To: <20240119060721.3734775-2-brianmlyles@gmail.com>
From: Brian Lyles <brianmlyles@gmail.com>
As with `git-rebase` and `git-am`, `git-cherry-pick` can result in a
commit being made redundant if the content from the picked commit is
already present in the target history. However, `git-cherry-pick` does
not have the same options available that `git-rebase` and `git-am` have.
There are three things that can be done with these redundant commits:
drop them, keep them, or have the cherry-pick stop and wait for the user
to take an action. `git-rebase` has the `--empty` option added in commit
e98c4269c8 (rebase (interactive-backend): fix handling of commits that
become empty, 2020-02-15), which handles all three of these scenarios.
Similarly, `git-am` got its own `--empty` in 7c096b8d61 (am: support
--empty=<option> to handle empty patches, 2021-12-09).
`git-cherry-pick`, on the other hand, only supports two of the three
possiblities: Keep the redundant commits via `--keep-redundant-commits`,
or have the cherry-pick fail by not specifying that option. There is no
way to automatically drop redundant commits.
In order to bring `git-cherry-pick` more in-line with `git-rebase` and
`git-am`, this commit adds an `--empty` option to `git-cherry-pick`. It
has the same three options (keep, drop, and stop), and largely behaves
the same. The notable difference is that for `git-cherry-pick`, the
default will be `stop`, which maintains the current behavior when the
option is not specified.
The `--keep-redundant-commits` option will be documented as a deprecated
synonym of `--empty=keep`, and will be supported for backwards
compatibility for the time being.
Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
---
Documentation/git-cherry-pick.txt | 28 ++++++++++++++++++-------
builtin/revert.c | 35 ++++++++++++++++++++++++++++++-
sequencer.c | 6 ++++++
t/t3505-cherry-pick-empty.sh | 26 ++++++++++++++++++++++-
4 files changed, 86 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 806295a730..8c20a10d4b 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -132,23 +132,37 @@ effect to your index in a row.
keeps commits that were initially empty (i.e. the commit recorded the
same tree as its parent). Commits which are made empty due to a
previous commit will cause the cherry-pick to fail. To force the
- inclusion of those commits use `--keep-redundant-commits`.
+ inclusion of those commits use `--empty=keep`.
--allow-empty-message::
By default, cherry-picking a commit with an empty message will fail.
This option overrides that behavior, allowing commits with empty
messages to be cherry picked.
---keep-redundant-commits::
- If a commit being cherry picked duplicates a commit already in the
- current history, it will become empty. By default these
- redundant commits cause `cherry-pick` to stop so the user can
- examine the commit. This option overrides that behavior and
- creates an empty commit object. Note that use of this option only
+--empty=(stop|drop|keep)::
+ How to handle commits being cherry-picked that are redundant with
+ changes already in the current history.
++
+--
+`stop`;;
+ The cherry-pick will stop when the empty commit is applied, allowing
+ you to examine the commit. This is the default behavior.
+`drop`;;
+ The empty commit will be dropped.
+`keep`;;
+ The empty commit will be kept. Note that use of this option only
results in an empty commit when the commit was not initially empty,
but rather became empty due to a previous commit. Commits that were
initially empty will cause the cherry-pick to fail. To force the
inclusion of those commits use `--allow-empty`.
+--
++
+Note that commits which start empty will cause the cherry-pick to fail (unless
+`--allow-empty` is specified).
++
+
+--keep-redundant-commits::
+ Deprecated synonym for `--empty=keep`.
--strategy=<strategy>::
Use the given merge strategy. Should only be used once.
diff --git a/builtin/revert.c b/builtin/revert.c
index b2cfde7a87..1491c45e26 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -45,6 +45,30 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
}
+enum empty_action {
+ STOP_ON_EMPTY_COMMIT = 0, /* output errors and stop in the middle of a cherry-pick */
+ DROP_EMPTY_COMMIT, /* skip with a notice message */
+ KEEP_EMPTY_COMMIT, /* keep recording as empty commits */
+};
+
+static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
+{
+ int *opt_value = opt->value;
+
+ BUG_ON_OPT_NEG(unset);
+
+ if (!strcmp(arg, "stop"))
+ *opt_value = STOP_ON_EMPTY_COMMIT;
+ else if (!strcmp(arg, "drop"))
+ *opt_value = DROP_EMPTY_COMMIT;
+ else if (!strcmp(arg, "keep"))
+ *opt_value = KEEP_EMPTY_COMMIT;
+ else
+ return error(_("invalid value for '%s': '%s'"), "--empty", arg);
+
+ return 0;
+}
+
static int option_parse_m(const struct option *opt,
const char *arg, int unset)
{
@@ -87,6 +111,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
const char *me = action_name(opts);
const char *cleanup_arg = NULL;
+ enum empty_action empty_opt;
int cmd = 0;
struct option base_options[] = {
OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
@@ -116,7 +141,10 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
OPT_BOOL(0, "ff", &opts->allow_ff, N_("allow fast-forward")),
OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
- OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
+ OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("deprecated: use --empty=keep instead")),
+ OPT_CALLBACK_F(0, "empty", &empty_opt, "(stop|drop|keep)",
+ N_("how to handle commits that become empty"),
+ PARSE_OPT_NONEG, parse_opt_empty),
OPT_END(),
};
options = parse_options_concat(options, cp_extra);
@@ -136,6 +164,11 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
prepare_repo_settings(the_repository);
the_repository->settings.command_requires_full_index = 0;
+ if (opts->action == REPLAY_PICK) {
+ opts->drop_redundant_commits = (empty_opt == DROP_EMPTY_COMMIT);
+ opts->keep_redundant_commits = opts->keep_redundant_commits || (empty_opt == KEEP_EMPTY_COMMIT);
+ }
+
if (cleanup_arg) {
opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
opts->explicit_cleanup = 1;
diff --git a/sequencer.c b/sequencer.c
index 582bde8d46..c49c27c795 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2934,6 +2934,9 @@ static int populate_opts_cb(const char *key, const char *value,
else if (!strcmp(key, "options.allow-empty-message"))
opts->allow_empty_message =
git_config_bool_or_int(key, value, ctx->kvi, &error_flag);
+ else if (!strcmp(key, "options.drop-redundant-commits"))
+ opts->drop_redundant_commits =
+ git_config_bool_or_int(key, value, ctx->kvi, &error_flag);
else if (!strcmp(key, "options.keep-redundant-commits"))
opts->keep_redundant_commits =
git_config_bool_or_int(key, value, ctx->kvi, &error_flag);
@@ -3478,6 +3481,9 @@ static int save_opts(struct replay_opts *opts)
if (opts->allow_empty_message)
res |= git_config_set_in_file_gently(opts_file,
"options.allow-empty-message", "true");
+ if (opts->drop_redundant_commits)
+ res |= git_config_set_in_file_gently(opts_file,
+ "options.drop-redundant-commits", "true");
if (opts->keep_redundant_commits)
res |= git_config_set_in_file_gently(opts_file,
"options.keep-redundant-commits", "true");
diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index 6adfd25351..ae0cf7886a 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -89,7 +89,7 @@ test_expect_success 'cherry-pick a commit that becomes no-op (prep)' '
git commit -m "add file2 on the side"
'
-test_expect_success 'cherry-pick a no-op without --keep-redundant' '
+test_expect_success 'cherry-pick a no-op with neither --keep-redundant nor --empty' '
git reset --hard &&
git checkout fork^0 &&
test_must_fail git cherry-pick main
@@ -104,4 +104,28 @@ test_expect_success 'cherry-pick a no-op with --keep-redundant' '
test_cmp expect actual
'
+test_expect_success 'cherry-pick a no-op with --empty=ask' '
+ git reset --hard &&
+ git checkout fork^0 &&
+ test_must_fail git cherry-pick --empty=ask main
+'
+
+test_expect_success 'cherry-pick a no-op with --empty=drop' '
+ git reset --hard &&
+ git checkout fork^0 &&
+ git cherry-pick --empty=drop main &&
+ git show -s --format=%s >actual &&
+ echo "add file2 on the side" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick a no-op with --empty=keep' '
+ git reset --hard &&
+ git checkout fork^0 &&
+ git cherry-pick --empty=keep main &&
+ git show -s --format=%s >actual &&
+ echo "add file2 on main" >expect &&
+ test_cmp expect actual
+'
+
test_done
--
2.41.0
^ 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