* [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro
From: Taylor Blau @ 2023-09-05 20:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1693946195.git.me@ttaylorr.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 478fab96c9..6110598a69 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -26,8 +26,6 @@
#define LOOSEN_UNREACHABLE 2
#define PACK_CRUFT 4
-#define DELETE_PACK 1
-
static int pack_everything;
static int delta_base_offset = 1;
static int pack_kept_objects = -1;
@@ -96,6 +94,10 @@ static int repack_config(const char *var, const char *value,
struct existing_packs {
struct string_list kept_packs;
+ /*
+ * for both non_kept_packs, and cruft_packs, a non-NULL
+ * 'util' field indicates the pack should be deleted.
+ */
struct string_list non_kept_packs;
struct string_list cruft_packs;
};
@@ -130,7 +132,7 @@ static void mark_packs_for_deletion_1(struct string_list *names,
* (if `-d` was given).
*/
if (!string_list_has_string(names, sha1))
- item->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);
+ item->util = (void*)1;
}
}
@@ -158,7 +160,7 @@ static void remove_redundant_packs_1(struct string_list *packs)
{
struct string_list_item *item;
for_each_string_list_item(item, packs) {
- if (!((uintptr_t)item->util & DELETE_PACK))
+ if (!item->util)
continue;
remove_redundant_pack(packdir, item->string);
}
@@ -695,20 +697,20 @@ static void midx_included_packs(struct string_list *include,
for_each_string_list_item(item, &existing->cruft_packs) {
/*
- * no need to check DELETE_PACK, since we're not
- * doing an ALL_INTO_ONE repack
+ * no need to check for deleted packs, since we're
+ * not doing an ALL_INTO_ONE repack
*/
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
} else {
for_each_string_list_item(item, &existing->non_kept_packs) {
- if ((uintptr_t)item->util & DELETE_PACK)
+ if (item->util)
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
for_each_string_list_item(item, &existing->cruft_packs) {
- if ((uintptr_t)item->util & DELETE_PACK)
+ if (item->util)
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
--
2.42.0.119.gca7d13e7bf
^ permalink raw reply related
* [PATCH 6/8] builtin/repack.c: store existing cruft packs separately
From: Taylor Blau @ 2023-09-05 20:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1693946195.git.me@ttaylorr.com>
When repacking with the `--write-midx` option, we invoke the function
`midx_included_packs()` in order to produce the list of packs we want to
include in the resulting MIDX.
This list is comprised of:
- existing .keep packs
- any pack(s) which were written earlier in the same process
- any unchanged packs when doing a `--geometric` repack
- any cruft packs
Prior to this patch, we stored pre-existing cruft and non-cruft packs
together (provided those packs are non-kept). This meant we needed an
additional bit to indicate which non-kept pack(s) were cruft versus
those that aren't.
But alternatively we can store cruft packs in a separate list, avoiding
the need for this extra bit, and simplifying the code below.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 3f0789ff89..478fab96c9 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -27,7 +27,6 @@
#define PACK_CRUFT 4
#define DELETE_PACK 1
-#define CRUFT_PACK 2
static int pack_everything;
static int delta_base_offset = 1;
@@ -98,16 +97,18 @@ static int repack_config(const char *var, const char *value,
struct existing_packs {
struct string_list kept_packs;
struct string_list non_kept_packs;
+ struct string_list cruft_packs;
};
#define EXISTING_PACKS_INIT { \
.kept_packs = STRING_LIST_INIT_DUP, \
.non_kept_packs = STRING_LIST_INIT_DUP, \
+ .cruft_packs = STRING_LIST_INIT_DUP, \
}
static int has_existing_non_kept_packs(const struct existing_packs *existing)
{
- return existing->non_kept_packs.nr;
+ return existing->non_kept_packs.nr || existing->cruft_packs.nr;
}
static void mark_packs_for_deletion_1(struct string_list *names,
@@ -138,6 +139,7 @@ static void mark_packs_for_deletion(struct existing_packs *existing,
{
mark_packs_for_deletion_1(names, &existing->non_kept_packs);
+ mark_packs_for_deletion_1(names, &existing->cruft_packs);
}
static void remove_redundant_pack(const char *dir_name, const char *base_name)
@@ -165,12 +167,14 @@ static void remove_redundant_packs_1(struct string_list *packs)
static void remove_redundant_existing_packs(struct existing_packs *existing)
{
remove_redundant_packs_1(&existing->non_kept_packs);
+ remove_redundant_packs_1(&existing->cruft_packs);
}
static void existing_packs_release(struct existing_packs *existing)
{
string_list_clear(&existing->kept_packs, 0);
string_list_clear(&existing->non_kept_packs, 0);
+ string_list_clear(&existing->cruft_packs, 0);
}
/*
@@ -204,12 +208,10 @@ static void collect_pack_filenames(struct existing_packs *existing,
if ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)
string_list_append(&existing->kept_packs, buf.buf);
- else {
- struct string_list_item *item;
- item = string_list_append(&existing->non_kept_packs, buf.buf);
- if (p->is_cruft)
- item->util = (void*)(uintptr_t)CRUFT_PACK;
- }
+ else if (p->is_cruft)
+ string_list_append(&existing->cruft_packs, buf.buf);
+ else
+ string_list_append(&existing->non_kept_packs, buf.buf);
}
string_list_sort(&existing->kept_packs);
@@ -691,14 +693,11 @@ static void midx_included_packs(struct string_list *include,
string_list_insert(include, strbuf_detach(&buf, NULL));
}
- for_each_string_list_item(item, &existing->non_kept_packs) {
- if (!((uintptr_t)item->util & CRUFT_PACK)) {
- /*
- * no need to check DELETE_PACK, since we're not
- * doing an ALL_INTO_ONE repack
- */
- continue;
- }
+ for_each_string_list_item(item, &existing->cruft_packs) {
+ /*
+ * no need to check DELETE_PACK, since we're not
+ * doing an ALL_INTO_ONE repack
+ */
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
} else {
@@ -707,6 +706,12 @@ static void midx_included_packs(struct string_list *include,
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
+
+ for_each_string_list_item(item, &existing->cruft_packs) {
+ if ((uintptr_t)item->util & DELETE_PACK)
+ continue;
+ string_list_insert(include, xstrfmt("%s.idx", item->string));
+ }
}
}
@@ -836,6 +841,8 @@ static int write_cruft_pack(const struct pack_objects_args *args,
fprintf(in, "%s-%s.pack\n", pack_prefix, item->string);
for_each_string_list_item(item, &existing->non_kept_packs)
fprintf(in, "-%s.pack\n", item->string);
+ for_each_string_list_item(item, &existing->cruft_packs)
+ fprintf(in, "-%s.pack\n", item->string);
for_each_string_list_item(item, &existing->kept_packs)
fprintf(in, "%s.pack\n", item->string);
fclose(in);
--
2.42.0.119.gca7d13e7bf
^ permalink raw reply related
* [PATCH 4/8] builtin/repack.c: extract redundant pack cleanup for existing packs
From: Taylor Blau @ 2023-09-05 20:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1693946195.git.me@ttaylorr.com>
To remove redundant packs at the end of a repacking operation, Git uses
its `remove_redundant_pack()` function in a loop over the set of
pre-existing, non-kept packs.
In a later commit, we will split this list into two, one for
pre-existing cruft pack(s), and another for non-cruft pack(s). Prepare
for this by factoring out the routine to loop over and delete redundant
packs into its own function.
Instead of calling `remove_redundant_pack()` directly, we now will call
`remove_redundant_existing_packs()`, which itself dispatches a call to
`remove_redundant_packs_1()`. Note that the geometric repacking code
will still call `remove_redundant_pack()` directly, but see the previous
commit for more details.
Having `remove_redundant_packs_1()` exist as a separate function may
seem like overkill in this patch. However, a later patch will call
`remove_redundant_packs_1()` once over two separate lists, so this
refactoring sets us up for that.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 45 ++++++++++++++++++++++++++++-----------------
1 file changed, 28 insertions(+), 17 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index d3e6326bb9..f6717e334c 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -135,6 +135,33 @@ static void mark_packs_for_deletion(struct existing_packs *existing,
mark_packs_for_deletion_1(names, &existing->non_kept_packs);
}
+static void remove_redundant_pack(const char *dir_name, const char *base_name)
+{
+ struct strbuf buf = STRBUF_INIT;
+ struct multi_pack_index *m = get_local_multi_pack_index(the_repository);
+ strbuf_addf(&buf, "%s.pack", base_name);
+ if (m && midx_contains_pack(m, buf.buf))
+ clear_midx_file(the_repository);
+ strbuf_insertf(&buf, 0, "%s/", dir_name);
+ unlink_pack_path(buf.buf, 1);
+ strbuf_release(&buf);
+}
+
+static void remove_redundant_packs_1(struct string_list *packs)
+{
+ struct string_list_item *item;
+ for_each_string_list_item(item, packs) {
+ if (!((uintptr_t)item->util & DELETE_PACK))
+ continue;
+ remove_redundant_pack(packdir, item->string);
+ }
+}
+
+static void remove_redundant_existing_packs(struct existing_packs *existing)
+{
+ remove_redundant_packs_1(&existing->non_kept_packs);
+}
+
static void existing_packs_release(struct existing_packs *existing)
{
string_list_clear(&existing->kept_packs, 0);
@@ -184,18 +211,6 @@ static void collect_pack_filenames(struct existing_packs *existing,
strbuf_release(&buf);
}
-static void remove_redundant_pack(const char *dir_name, const char *base_name)
-{
- struct strbuf buf = STRBUF_INIT;
- struct multi_pack_index *m = get_local_multi_pack_index(the_repository);
- strbuf_addf(&buf, "%s.pack", base_name);
- if (m && midx_contains_pack(m, buf.buf))
- clear_midx_file(the_repository);
- strbuf_insertf(&buf, 0, "%s/", dir_name);
- unlink_pack_path(buf.buf, 1);
- strbuf_release(&buf);
-}
-
static void prepare_pack_objects(struct child_process *cmd,
const struct pack_objects_args *args,
const char *out)
@@ -1221,11 +1236,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (delete_redundant) {
int opts = 0;
- for_each_string_list_item(item, &existing.non_kept_packs) {
- if (!((uintptr_t)item->util & DELETE_PACK))
- continue;
- remove_redundant_pack(packdir, item->string);
- }
+ remove_redundant_existing_packs(&existing);
if (geometry.split_factor)
geometry_remove_redundant_packs(&geometry, &names,
--
2.42.0.119.gca7d13e7bf
^ permalink raw reply related
* [PATCH 1/8] builtin/repack.c: extract structure to store existing packs
From: Taylor Blau @ 2023-09-05 20:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1693946195.git.me@ttaylorr.com>
The repack machinery needs to keep track of which packfiles were present
in the repository at the beginning of a repack, segmented by whether or
not each pack is marked as kept.
The names of these packs are stored in two `string_list`s, corresponding
to kept- and non-kept packs, respectively. As a consequence, many
functions within the repack code need to take both `string_list`s as
arguments, leading to code like this:
ret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,
cruft_expiration, &names,
&existing_nonkept_packs, /* <- */
&existing_kept_packs); /* <- */
Wrap up this pair of `string_list`s into a single structure that stores
both. This saves us from having to pass both string lists separately,
and prepares for adding additional fields to this structure.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 90 ++++++++++++++++++++++++++----------------------
1 file changed, 49 insertions(+), 41 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 2b43a5be08..c3ab89912e 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -95,14 +95,29 @@ static int repack_config(const char *var, const char *value,
return git_default_config(var, value, ctx, cb);
}
+struct existing_packs {
+ struct string_list kept_packs;
+ struct string_list non_kept_packs;
+};
+
+#define EXISTING_PACKS_INIT { \
+ .kept_packs = STRING_LIST_INIT_DUP, \
+ .non_kept_packs = STRING_LIST_INIT_DUP, \
+}
+
+static void existing_packs_release(struct existing_packs *existing)
+{
+ string_list_clear(&existing->kept_packs, 0);
+ string_list_clear(&existing->non_kept_packs, 0);
+}
+
/*
- * Adds all packs hex strings (pack-$HASH) to either fname_nonkept_list
- * or fname_kept_list based on whether each pack has a corresponding
+ * Adds all packs hex strings (pack-$HASH) to either packs->non_kept
+ * or packs->kept based on whether each pack has a corresponding
* .keep file or not. Packs without a .keep file are not to be kept
* if we are going to pack everything into one file.
*/
-static void collect_pack_filenames(struct string_list *fname_nonkept_list,
- struct string_list *fname_kept_list,
+static void collect_pack_filenames(struct existing_packs *existing,
const struct string_list *extra_keep)
{
struct packed_git *p;
@@ -126,16 +141,16 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,
strbuf_strip_suffix(&buf, ".pack");
if ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)
- string_list_append(fname_kept_list, buf.buf);
+ string_list_append(&existing->kept_packs, buf.buf);
else {
struct string_list_item *item;
- item = string_list_append(fname_nonkept_list, buf.buf);
+ item = string_list_append(&existing->non_kept_packs, buf.buf);
if (p->is_cruft)
item->util = (void*)(uintptr_t)CRUFT_PACK;
}
}
- string_list_sort(fname_kept_list);
+ string_list_sort(&existing->kept_packs);
strbuf_release(&buf);
}
@@ -327,7 +342,7 @@ static int geometry_cmp(const void *va, const void *vb)
}
static void init_pack_geometry(struct pack_geometry *geometry,
- struct string_list *existing_kept_packs,
+ struct existing_packs *existing,
const struct pack_objects_args *args)
{
struct packed_git *p;
@@ -344,23 +359,24 @@ static void init_pack_geometry(struct pack_geometry *geometry,
if (!pack_kept_objects) {
/*
- * Any pack that has its pack_keep bit set will appear
- * in existing_kept_packs below, but this saves us from
- * doing a more expensive check.
+ * Any pack that has its pack_keep bit set will
+ * appear in existing->kept_packs below, but
+ * this saves us from doing a more expensive
+ * check.
*/
if (p->pack_keep)
continue;
/*
- * The pack may be kept via the --keep-pack option;
- * check 'existing_kept_packs' to determine whether to
- * ignore it.
+ * The pack may be kept via the --keep-pack
+ * option; check 'existing->kept_packs' to
+ * determine whether to ignore it.
*/
strbuf_reset(&buf);
strbuf_addstr(&buf, pack_basename(p));
strbuf_strip_suffix(&buf, ".pack");
- if (string_list_has_string(existing_kept_packs, buf.buf))
+ if (string_list_has_string(&existing->kept_packs, buf.buf))
continue;
}
if (p->is_cruft)
@@ -565,14 +581,13 @@ static void midx_snapshot_refs(struct tempfile *f)
}
static void midx_included_packs(struct string_list *include,
- struct string_list *existing_nonkept_packs,
- struct string_list *existing_kept_packs,
+ struct existing_packs *existing,
struct string_list *names,
struct pack_geometry *geometry)
{
struct string_list_item *item;
- for_each_string_list_item(item, existing_kept_packs)
+ for_each_string_list_item(item, &existing->kept_packs)
string_list_insert(include, xstrfmt("%s.idx", item->string));
for_each_string_list_item(item, names)
string_list_insert(include, xstrfmt("pack-%s.idx", item->string));
@@ -600,7 +615,7 @@ static void midx_included_packs(struct string_list *include,
string_list_insert(include, strbuf_detach(&buf, NULL));
}
- for_each_string_list_item(item, existing_nonkept_packs) {
+ for_each_string_list_item(item, &existing->non_kept_packs) {
if (!((uintptr_t)item->util & CRUFT_PACK)) {
/*
* no need to check DELETE_PACK, since we're not
@@ -611,7 +626,7 @@ static void midx_included_packs(struct string_list *include,
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
} else {
- for_each_string_list_item(item, existing_nonkept_packs) {
+ for_each_string_list_item(item, &existing->non_kept_packs) {
if ((uintptr_t)item->util & DELETE_PACK)
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
@@ -700,8 +715,7 @@ static int write_cruft_pack(const struct pack_objects_args *args,
const char *pack_prefix,
const char *cruft_expiration,
struct string_list *names,
- struct string_list *existing_packs,
- struct string_list *existing_kept_packs)
+ struct existing_packs *existing)
{
struct child_process cmd = CHILD_PROCESS_INIT;
struct strbuf line = STRBUF_INIT;
@@ -744,9 +758,9 @@ static int write_cruft_pack(const struct pack_objects_args *args,
in = xfdopen(cmd.in, "w");
for_each_string_list_item(item, names)
fprintf(in, "%s-%s.pack\n", pack_prefix, item->string);
- for_each_string_list_item(item, existing_packs)
+ for_each_string_list_item(item, &existing->non_kept_packs)
fprintf(in, "-%s.pack\n", item->string);
- for_each_string_list_item(item, existing_kept_packs)
+ for_each_string_list_item(item, &existing->kept_packs)
fprintf(in, "%s.pack\n", item->string);
fclose(in);
@@ -778,8 +792,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
struct child_process cmd = CHILD_PROCESS_INIT;
struct string_list_item *item;
struct string_list names = STRING_LIST_INIT_DUP;
- struct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;
- struct string_list existing_kept_packs = STRING_LIST_INIT_DUP;
+ struct existing_packs existing = EXISTING_PACKS_INIT;
struct pack_geometry geometry = { 0 };
struct strbuf line = STRBUF_INIT;
struct tempfile *refs_snapshot = NULL;
@@ -915,13 +928,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
packtmp_name = xstrfmt(".tmp-%d-pack", (int)getpid());
packtmp = mkpathdup("%s/%s", packdir, packtmp_name);
- collect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,
- &keep_pack_list);
+ collect_pack_filenames(&existing, &keep_pack_list);
if (geometry.split_factor) {
if (pack_everything)
die(_("options '%s' and '%s' cannot be used together"), "--geometric", "-A/-a");
- init_pack_geometry(&geometry, &existing_kept_packs, &po_args);
+ init_pack_geometry(&geometry, &existing, &po_args);
split_pack_geometry(&geometry);
}
@@ -965,7 +977,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (pack_everything & ALL_INTO_ONE) {
repack_promisor_objects(&po_args, &names);
- if (existing_nonkept_packs.nr && delete_redundant &&
+ if (existing.non_kept_packs.nr && delete_redundant &&
!(pack_everything & PACK_CRUFT)) {
for_each_string_list_item(item, &names) {
strvec_pushf(&cmd.args, "--keep-pack=%s-%s.pack",
@@ -1054,8 +1066,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
ret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,
cruft_expiration, &names,
- &existing_nonkept_packs,
- &existing_kept_packs);
+ &existing);
if (ret)
goto cleanup;
@@ -1086,8 +1097,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
pack_prefix,
NULL,
&names,
- &existing_nonkept_packs,
- &existing_kept_packs);
+ &existing);
if (ret)
goto cleanup;
}
@@ -1133,7 +1143,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (delete_redundant && pack_everything & ALL_INTO_ONE) {
const int hexsz = the_hash_algo->hexsz;
- for_each_string_list_item(item, &existing_nonkept_packs) {
+ for_each_string_list_item(item, &existing.non_kept_packs) {
char *sha1;
size_t len = strlen(item->string);
if (len < hexsz)
@@ -1152,8 +1162,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (write_midx) {
struct string_list include = STRING_LIST_INIT_NODUP;
- midx_included_packs(&include, &existing_nonkept_packs,
- &existing_kept_packs, &names, &geometry);
+ midx_included_packs(&include, &existing, &names, &geometry);
ret = write_midx_included_packs(&include, &geometry,
refs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,
@@ -1172,7 +1181,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (delete_redundant) {
int opts = 0;
- for_each_string_list_item(item, &existing_nonkept_packs) {
+ for_each_string_list_item(item, &existing.non_kept_packs) {
if (!((uintptr_t)item->util & DELETE_PACK))
continue;
remove_redundant_pack(packdir, item->string);
@@ -1193,7 +1202,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
strbuf_strip_suffix(&buf, ".pack");
if ((p->pack_keep) ||
- (string_list_has_string(&existing_kept_packs,
+ (string_list_has_string(&existing.kept_packs,
buf.buf)))
continue;
@@ -1224,8 +1233,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
cleanup:
string_list_clear(&names, 1);
- string_list_clear(&existing_nonkept_packs, 0);
- string_list_clear(&existing_kept_packs, 0);
+ existing_packs_release(&existing);
free_pack_geometry(&geometry);
return ret;
--
2.42.0.119.gca7d13e7bf
^ permalink raw reply related
* [PATCH 3/8] builtin/repack.c: extract redundant pack cleanup for --geometric
From: Taylor Blau @ 2023-09-05 20:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1693946195.git.me@ttaylorr.com>
To reduce the complexity of the already quite-long `cmd_repack()`
implementation, extract out the parts responsible for deleting redundant
packs from a geometric repack out into its own sub-routine.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 52 +++++++++++++++++++++++++++---------------------
1 file changed, 29 insertions(+), 23 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 708556836e..d3e6326bb9 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -540,6 +540,32 @@ static struct packed_git *get_preferred_pack(struct pack_geometry *geometry)
return NULL;
}
+static void geometry_remove_redundant_packs(struct pack_geometry *geometry,
+ struct string_list *names,
+ struct existing_packs *existing)
+{
+ struct strbuf buf = STRBUF_INIT;
+ uint32_t i;
+
+ for (i = 0; i < geometry->split; i++) {
+ struct packed_git *p = geometry->pack[i];
+ if (string_list_has_string(names, hash_to_hex(p->hash)))
+ continue;
+
+ strbuf_reset(&buf);
+ strbuf_addstr(&buf, pack_basename(p));
+ strbuf_strip_suffix(&buf, ".pack");
+
+ if ((p->pack_keep) ||
+ (string_list_has_string(&existing->kept_packs, buf.buf)))
+ continue;
+
+ remove_redundant_pack(packdir, buf.buf);
+ }
+
+ strbuf_release(&buf);
+}
+
static void free_pack_geometry(struct pack_geometry *geometry)
{
if (!geometry)
@@ -1201,29 +1227,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
remove_redundant_pack(packdir, item->string);
}
- if (geometry.split_factor) {
- struct strbuf buf = STRBUF_INIT;
-
- uint32_t i;
- for (i = 0; i < geometry.split; i++) {
- struct packed_git *p = geometry.pack[i];
- if (string_list_has_string(&names,
- hash_to_hex(p->hash)))
- continue;
-
- strbuf_reset(&buf);
- strbuf_addstr(&buf, pack_basename(p));
- strbuf_strip_suffix(&buf, ".pack");
-
- if ((p->pack_keep) ||
- (string_list_has_string(&existing.kept_packs,
- buf.buf)))
- continue;
-
- remove_redundant_pack(packdir, buf.buf);
- }
- strbuf_release(&buf);
- }
+ if (geometry.split_factor)
+ geometry_remove_redundant_packs(&geometry, &names,
+ &existing);
if (show_progress)
opts |= PRUNE_PACKED_VERBOSE;
prune_packed_objects(opts);
--
2.42.0.119.gca7d13e7bf
^ permalink raw reply related
* [PATCH 5/8] builtin/repack.c: extract `has_existing_non_kept_packs()`
From: Taylor Blau @ 2023-09-05 20:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1693946195.git.me@ttaylorr.com>
When there is:
- at least one pre-existing packfile (which is not marked as kept),
- repacking with the `-d` flag, and
- not doing a cruft repack
, then we pass a handful of additional options to the inner
`pack-objects` process, like `--unpack-unreachable`,
`--keep-unreachable`, and `--pack-loose-unreachable`, in addition to
marking any packs we just wrote for promisor remotes as kept in-core
(with `--keep-pack`, as opposed to the presence of a ".keep" file on
disk).
Because we store both cruft and non-cruft packs together in the same
`existing.non_kept_packs` list, it suffices to check its `nr` member to
see if it is zero or not.
But a following change will store cruft- and non-cruft packs separately,
meaning this check would break as a result. Prepare for this by
extracting this part of the check into a new helper function called
`has_existing_non_kept_packs()`.
This patch does not introduce any functional changes, but prepares us to
make a more isolated change in a subsequent patch.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index f6717e334c..3f0789ff89 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -105,6 +105,11 @@ struct existing_packs {
.non_kept_packs = STRING_LIST_INIT_DUP, \
}
+static int has_existing_non_kept_packs(const struct existing_packs *existing)
+{
+ return existing->non_kept_packs.nr;
+}
+
static void mark_packs_for_deletion_1(struct string_list *names,
struct string_list *list)
{
@@ -1048,7 +1053,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (pack_everything & ALL_INTO_ONE) {
repack_promisor_objects(&po_args, &names);
- if (existing.non_kept_packs.nr && delete_redundant &&
+ if (has_existing_non_kept_packs(&existing) &&
+ delete_redundant &&
!(pack_everything & PACK_CRUFT)) {
for_each_string_list_item(item, &names) {
strvec_pushf(&cmd.args, "--keep-pack=%s-%s.pack",
--
2.42.0.119.gca7d13e7bf
^ permalink raw reply related
* [PATCH 2/8] builtin/repack.c: extract marking packs for deletion
From: Taylor Blau @ 2023-09-05 20:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1693946195.git.me@ttaylorr.com>
At the end of a repack (when given `-d`), Git attempts to remove any
packs which have been made "redundant" as a result of the repacking
operation. For example, an all-into-one (`-A` or `-a`) repack makes
every pre-existing pack which is not marked as kept redundant. Geometric
repacks (with `--geometric=<n>`) make any packs which were rolled up
redundant, and so on.
But before deleting the set of packs we think are redundant, we first
check to see whether or not we just wrote a pack which is identical to
any one of the packs we were going to delete. When this is the case, Git
must avoid deleting that pack, since it matches a pack we just wrote
(so deleting it may cause the repository to become corrupt).
Right now we only process the list of non-kept packs in a single pass.
But a future change will split the existing non-kept packs further into
two lists: one for cruft packs, and another for non-cruft packs.
Factor out this routine to prepare for calling it twice on two separate
lists in a future patch.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 50 +++++++++++++++++++++++++++++++-----------------
1 file changed, 32 insertions(+), 18 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index c3ab89912e..708556836e 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -105,6 +105,36 @@ struct existing_packs {
.non_kept_packs = STRING_LIST_INIT_DUP, \
}
+static void mark_packs_for_deletion_1(struct string_list *names,
+ struct string_list *list)
+{
+ struct string_list_item *item;
+ const int hexsz = the_hash_algo->hexsz;
+
+ for_each_string_list_item(item, list) {
+ char *sha1;
+ size_t len = strlen(item->string);
+ if (len < hexsz)
+ continue;
+ sha1 = item->string + len - hexsz;
+ /*
+ * Mark this pack for deletion, which ensures that this
+ * pack won't be included in a MIDX (if `--write-midx`
+ * was given) and that we will actually delete this pack
+ * (if `-d` was given).
+ */
+ if (!string_list_has_string(names, sha1))
+ item->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);
+ }
+}
+
+static void mark_packs_for_deletion(struct existing_packs *existing,
+ struct string_list *names)
+
+{
+ mark_packs_for_deletion_1(names, &existing->non_kept_packs);
+}
+
static void existing_packs_release(struct existing_packs *existing)
{
string_list_clear(&existing->kept_packs, 0);
@@ -1141,24 +1171,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
}
/* End of pack replacement. */
- if (delete_redundant && pack_everything & ALL_INTO_ONE) {
- const int hexsz = the_hash_algo->hexsz;
- for_each_string_list_item(item, &existing.non_kept_packs) {
- char *sha1;
- size_t len = strlen(item->string);
- if (len < hexsz)
- continue;
- sha1 = item->string + len - hexsz;
- /*
- * Mark this pack for deletion, which ensures that this
- * pack won't be included in a MIDX (if `--write-midx`
- * was given) and that we will actually delete this pack
- * (if `-d` was given).
- */
- if (!string_list_has_string(&names, sha1))
- item->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);
- }
- }
+ if (delete_redundant && pack_everything & ALL_INTO_ONE)
+ mark_packs_for_deletion(&existing, &names);
if (write_midx) {
struct string_list include = STRING_LIST_INIT_NODUP;
--
2.42.0.119.gca7d13e7bf
^ permalink raw reply related
* [PATCH 0/8] repack: refactor pack snapshot-ing logic
From: Taylor Blau @ 2023-09-05 20:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
This series refactors some of the 'git repack' internals to keep track
of existing packs in a set of string lists stored in a single structure,
instead of passing around multiple string lists to different functions
throughout the various paths.
The result is that the interface around pack deletion, marking packs as
redundant, and handling the set of pre-existing packs (both kept and
non-kept) is significantly cleaner without introducing any functional
changes.
I didn't mean to produce so much churn when I started writing these
patches, which began as a simple effort to rename a couple of variables
for more consistency.
Thanks in advance for your review!
Taylor Blau (8):
builtin/repack.c: extract structure to store existing packs
builtin/repack.c: extract marking packs for deletion
builtin/repack.c: extract redundant pack cleanup for --geometric
builtin/repack.c: extract redundant pack cleanup for existing packs
builtin/repack.c: extract `has_existing_non_kept_packs()`
builtin/repack.c: store existing cruft packs separately
builtin/repack.c: drop `DELETE_PACK` macro
builtin/repack.c: extract common cruft pack loop
builtin/repack.c | 288 ++++++++++++++++++++++++++++-------------------
1 file changed, 174 insertions(+), 114 deletions(-)
--
2.42.0.119.gca7d13e7bf
^ permalink raw reply
* [PATCH 1/1] doc/diff-options: fix link to generating patch section
From: Sergey Organov @ 2023-09-05 17:52 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, John Cai
First, there is no need for conditional referencing, as all the files
that include "diff-options.txt" eventually include
"diff-generate-patch.txt" as well.
Next, when formatted as man-page, the section title is rendered
"GENERATING PATCH TEXT WITH -P" whereas reference still reads
"Generating patch text with -p", that is both inconsistent and makes
searching harder than it needs to be.
Fix the issues by just referring to the section, without custom
reference text, and then unconditionally.
Fixes: ebdc46c242 (docs: link generating patch sections)
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
Documentation/diff-options.txt | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 9f33f887711d..c07488b123c6 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -22,13 +22,7 @@ ifndef::git-format-patch[]
-p::
-u::
--patch::
- Generate patch (see section titled
-ifdef::git-log[]
-<<generate_patch_text_with_p, "Generating patch text with -p">>).
-endif::git-log[]
-ifndef::git-log[]
-"Generating patch text with -p").
-endif::git-log[]
+ Generate patch (see <<generate_patch_text_with_p>>).
ifdef::git-diff[]
This is the default.
endif::git-diff[]
--
2.25.1
^ permalink raw reply related
* git clone command leaves orphaned ssh process on system
From: Max Amelchenko @ 2023-09-05 19:06 UTC (permalink / raw)
To: git
What did you do before the bug happened? (Steps to reproduce your issue)
Run the command:
ps aux
Observe no ssh processes running on system.
Run git clone against a non-existent hostname:
git clone -v --depth=1 -b 3.23.66
ssh://*****@*****lab-prod.server.sim.cloud/terraform/modules/aws-eks
/tmp/dest
Observe the command fails with:
Could not resolve hostname *****lab-prod.server.sim.cloud: Name or
service not known
Run:
ps aux
Observe a defunct ssh process is left behind.
What did you expect to happen? (Expected behavior)
I expected the command to quit without leaving any processes behind.
What happened instead? (Actual behavior)
The command quit and left a defunct ssh process on the system.
What's different between what you expected and what actually happened?
I don't want zombie processes left after any git command (either failed or not).
Anything else you want to add:
These processes are zombie orphaned, meaning we're stuck with them
until system reboot (which is bad).
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.40.1
cpu: aarch64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
compiler info: gnuc: 7.3
libc info: glibc: 2.26
$SHELL (typically, interactive shell): <unset>
[Enabled Hooks]
not run from a git repository - no hooks to show
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Tao Klerks @ 2023-09-05 16:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Eric Sunshine, git, Johannes Schindelin, Taylor Blau,
Patrick Steinhardt
In-Reply-To: <xmqqmsy0slei.fsf@gitster.g>
On Tue, Sep 5, 2023 at 5:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> >> All correct. The per-worktree part of the repository data does live
> >> in a subdirectory of the ".git" directory and that was probably what
> >> Tao had in mind, though.
> >
> > That could be. I read Tao's explanation as meaning that people do this:
> >
> > git clone foo.git foo
> > cd foo
> > git worktree add bar
> > git worktree add baz
> >
> > rather than (perhaps) this:
> >
> > git clone foo.git foo
> > cd foo
> > git worktree add ../bar
> > git worktree add ../baz
>
> Ah, that reading does totally make sense.
>
Fwiw, Eric's reading was my intended one. The people I have spoken
with, as well as myself, have started using "git worktree" by doing
the former, and only later felt really transgressive when placing the
worktrees explicitly on a higher level, on equal footing with the
"main worktree". To me it seemed natural that the "nested worktrees"
approach was the expected one, as otherwise it gets even harder to
explain/justify the operational difference between the "main worktree"
and the other worktrees - then leading to the bare+worktrees approach
to eliminate that operational difference.
> But I am not sure it would lead to "we need to carefully protect the
> primary worktree", because it is rather obvious, especially if you
> bypass "git worktree remove" and use "rm -fr", you would lose
> everybody underneath if you remove the "foo" in the "worktrees are
> subdirectories of the primary" variant in the above examples.
Right, sorry, too many poorly-expressed thoughts crammed together.
We need to start carefully protecting main worktree *when we start to
get clever* and actually add the worktrees as siblings to the main
worktree. That protection is indeed "implicit" before you start using
"../"... but then you have other issues of
git-worktree-within-git-worktree confusion.
Is there a manual for "expected typical usage of git worktree" somewhere?
>
> Even though deriving the worktree(s) from a separate and protected
> bare repositories does protect you from total disaster caused by
> removing "rm -fr" and bypassing "git worktree remove", it still
> should be discouraged, as the per-worktree states left behind in the
> repository interfere with the operations in surviving worktrees.
Right, that's fine. Of course you're going to encourage deleting the
worktrees carefully... but equally of-course, some people *will* do
"rm -fr that-worktree-I-dont-know-how-to-clean", and when they do,
telling them "just 'git worktree repair'" is much easier than telling
them to "recover deleted files 'cause your local branches just
evaporated"
> Teaching folks not to do "rm -fr" would be the first step to a more
> pleasant end-user experience, I would think.
The less arcane trivia you *need* to teach users for them to be
effective, the better the experience is for everyone.
The fact that "deleting a standalone git repo only deletes what's in
that standalone git repo the way you've done your whole life, but in
this environment what look like multiple repos are actually
'worktrees', if you ever delete one your life *might*, if you choose
the wrong one, suddenly be very unpleasant" is arcane trivia, in my
opinion. Better to set things up so they *can't* shoot themselves in
the foot with a bullet of that caliber.
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Tao Klerks @ 2023-09-05 16:10 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Johannes Schindelin, Taylor Blau, Patrick Steinhardt
In-Reply-To: <CAPig+cTeQDMpWQ-zCf6i9H-yhrdCndX6gs67sypuqmHZZcHm7w@mail.gmail.com>
On Tue, Sep 5, 2023 at 2:26 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Mon, Sep 4, 2023 at 10:41 AM Tao Klerks <tao@klerks.biz> wrote:
> >
> > One problem that we found users to have, early on when we introduced
> > worktrees, was that it was not obvious to users that there was (or why
> > there was) a "main" worktree, containing the actual ".git" repo, and
> > "subsidiary" worktrees, in the same directory location. Git by default
> > makes worktrees *subfolders* of the initial/main worktree/folder, but
>
> This is not accurate. There is no default location for new worktrees;
> git-worktree creates the new worktree at the location specified by the
> user:
>
> git worktree add [<options>] <path> [<commit>]
>
> where <path> -- the only mandatory argument -- specifies the location.
>
Right - I know you *can* create worktrees in the "parent path", and
now that I revisit the doc I see there are even a couple examples that
do exactly that - but whenever I've talked with people who've tried
"git worktree add" independently, they ended up with the nested
worktrees and wondering why things work this weird way.
The idea that the "most trivial" example of creating a worktree would
be "git worktree add ../my_new_worktree" is, I believe, very
non-obvious.
The other thing that is I believe is non-obvious, in the current
solution, is that *if* you end up placing multiple worktrees at the
same level, *and* you end up using them "interchangeably" as you
presumably would in some or most scenarios, even though their *usage*
is identical, their "importance", or "lifetime" is very much not.
>
> It indeed was designed to work this way. It is perfectly legitimate to
> create worktrees attached to a bare repository[1].
>
Awesome, thx for confirming!
> in a bare repository
> scenario, the term "main worktree" refers to the bare repository, not
> to the "blessed" worktree containing the ".git/" directory (since
> there is no such worktree in this case).
Noted, thank you. We have been using the word "repository" vs worktree
- the (main) GITDIR is the repo, the worktrees are all just worktrees.
There is no "main worktree" in the way we talk about things, although
clearly the official nomenclature doesn't square with that, which I
might need to address at some point.
> Worktrees appear to be a red-herring. It's possible to reproduce this
> error without them. For instance:
>
> % git clone --bare --filter=blob:none
> https://github.com/ksylor/ohshitgit dangit_shared.git
> % git clone dangit_shared.git foop
> % cd foop
> % echo nothing >nothing
> % git add nothing
> % git commit -m nothing
> fatal: unable to read dbbb0682a7690b62ccf51b2a8648fa71ac671348
> % git push origin master
> % cd ../dangit_shared.git
> % git gc
> ...
> warning: Failed to write bitmap index. Packfile doesn't have full
> closure (object bf86ed1b2602ac3a8d4724bcdf6707b156673aac is missing)
> fatal: failed to write bitmap index
> fatal: failed to run repack
>
Hmm, I don't really understand what happened there, but it looks to me
like you went *much* further off the beaten path by cloning from a
partial clone. Afaik that's hard-not-supported...?
>
> The former, meaning that your setup should be supported. Citing
> documentation for `core.bare`:
>
> If true this repository is assumed to be bare and has no working
> directory associated with it. If this is the case a number of
> commands that require a working directory will be disabled, such
> as git-add(1) or git-merge(1).
>
Thanks again!
>
> `--separate-git-dir` predates multiple-worktree support by several
> years and is distinct in purpose from --bare and multiple-worktrees
> (in fact, a couple somewhat recent fixes [2,3] were needed to prevent
> --separate-git-dir from breaking worktree administrative data). My
> understand from scanning history is that --separate-git-dir was
> introduced in aid of submodule support and perhaps other use-cases.
OK, so if it is legitimate as-is... why doesn't it set
"core.worktree"? At least that way there'd be a solid two-way
reference like with "git worktree" worktrees.
Or is the *point* of the submodule and/or other use-cases that the
gitdir can't "know" its worktree??
> git-new-workdir predates git-worktree by quite a few years and, as I
> understand it, remains in-tree because it fills a niche not entirely
> filled by git-worktree.
OK, I'll keep my nose out of this one entirely either way, thx :)
^ permalink raw reply
* Re: [PATCH v2 02/10] merge: simplify parsing of "-n" option
From: Jeff King @ 2023-09-05 6:43 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano
In-Reply-To: <75100a68-78d1-b22c-0497-36548c518b7b@web.de>
On Sat, Sep 02, 2023 at 08:20:28AM +0200, René Scharfe wrote:
> > static struct option builtin_merge_options[] = {
> > - OPT_CALLBACK_F('n', NULL, NULL, NULL,
> > - N_("do not show a diffstat at the end of the merge"),
> > - PARSE_OPT_NOARG, option_parse_n),
> > + OPT_SET_INT('n', NULL, &show_diffstat,
> > + N_("do not show a diffstat at the end of the merge"), 0),
> > OPT_BOOL(0, "stat", &show_diffstat,
> > N_("show a diffstat at the end of the merge")),
>
> Makes it easier to see that we can replace the two complementary
> definitions with a single one:
>
> OPT_NEGBIT('n', "no-stat",
> N_("do not show a diffstat at the end of the merge"), 1),
>
> Which is a separate topic, of course. And if we did that, however, ...
Ah, I thought we had a "reverse bool" of some kind, but I couldn't find
it. NEGBIT was what I was looking for. But yeah, I agree it gets more
complicated with the various aliases. I think what I have here is a good
stopping point for this series, but if you want to go further on it, be
my guest. :)
-Peff
^ permalink raw reply
* Re: [PATCH v3 6/7] rebase --continue: refuse to commit after failed command
From: Phillip Wood @ 2023-09-05 15:25 UTC (permalink / raw)
To: Johannes Schindelin, Phillip Wood
Cc: Phillip Wood via GitGitGadget, git, Junio C Hamano, Stefan Haller,
Eric Sunshine
In-Reply-To: <87fbc8c9-f42b-b374-fee1-57c58f5e8fc0@gmx.de>
Hi Johannes
On 05/09/2023 12:17, Johannes Schindelin wrote:
> A further contributing factor for my misunderstading was the slightly
> convoluted logic where `is_clean` is set to true if there are _not_ any
> uncommitted changes, and then we ask if `is_clean` is _not_ true. Reminds
> me of Smullyan's Knights & Knaves [*1*].
I agree 'is_clean' is confusing (I have the same problem with
merge_recursive() and friends where a return value of zero means that
there were conflicts)
> With your patch, there are now four users of the `is_clean` value, and
> all but one of them ask for the negated value.
>
> It's not really the responsibility of this patch series, but I could
> imagine that it would be nicer to future readers if a patch was added that
> would invert the meaning of that variable and rename it to
> `needs_committing`. At least to me, that would make the intention of the
> code eminently clearer.
Inverting and renaming 'is_clean' is a good idea, I might leave it to a
follow up series though.
Best Wishes
Phillip
> Ciao,
> Johannes
>
> Footnote *1*: https://en.wikipedia.org/wiki/Knights_and_Knaves
^ permalink raw reply
* Re: [PATCH] var: avoid a segmentation fault when `HOME` is unset
From: Johannes Schindelin @ 2023-09-05 10:58 UTC (permalink / raw)
To: brian m. carlson; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <ZPY9j0XDt/VVMnhG@tapette.crustytoothpaste.net>
Hi brian,
On Mon, 4 Sep 2023, brian m. carlson wrote:
> On 2023-09-04 at 06:21:26, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The code introduced in 576a37fccbf (var: add attributes files locations,
> > 2023-06-27) paid careful attention to use `xstrdup()` for pointers known
> > never to be `NULL`, and `xstrdup_or_null()` otherwise.
> >
> > One spot was missed, though: `git_attr_global_file()` can return `NULL`,
> > when the `HOME` variable is not set (and neither `XDG_CONFIG_HOME`), a
> > scenario not too uncommon in certain server scenarios.
> >
> > Fix this, and add a test case to avoid future regressions.
>
> Looks good to me.
Thank you for the review.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v3 6/7] rebase --continue: refuse to commit after failed command
From: Johannes Schindelin @ 2023-09-05 11:17 UTC (permalink / raw)
To: Phillip Wood
Cc: Phillip Wood via GitGitGadget, git, Junio C Hamano, Stefan Haller,
Eric Sunshine, Glen Choo
In-Reply-To: <02c28b26-4658-43c8-b1d1-7f1e09bda609@gmail.com>
Hi Phillip,
On Mon, 4 Sep 2023, Phillip Wood wrote:
> On 23/08/2023 10:01, Johannes Schindelin wrote:
>
> > On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote:
> >
> > > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> > >
> > > If a commit cannot be picked because it would overwrite an untracked
> > > file then "git rebase --continue" should refuse to commit any staged
> > > changes as the commit was not picked. This is implemented by refusing to
> > > commit if the message file is missing. The message file is chosen for
> > > this check because it is only written when "git rebase" stops for the
> > > user to resolve merge conflicts.
> > >
> > > Existing commands that refuse to commit staged changes when continuing
> > > such as a failed "exec" rely on checking for the absence of the author
> > > script in run_git_commit(). This prevents the staged changes from being
> > > committed but prints
> > >
> > > error: could not open '.git/rebase-merge/author-script' for
> > > reading
> > >
> > > before the message about not being able to commit. This is confusing to
> > > users and so checking for the message file instead improves the user
> > > experience. The existing test for refusing to commit after a failed exec
> > > is updated to check that we do not print the error message about a
> > > missing author script anymore.
> >
> > I am delighted to see an improvement of the user experience!
> >
> > However, I could imagine that users would still be confused when seeing
> > the advice about staged changes, even if nothing was staged at all.
>
> If nothing is staged then this message wont trigger because is_clean will be
> false.
Ah. I managed to get confused by the first sentence of the commit message
already. You clearly talk about "any staged changes". As in "*iff* there
are any staged changes". Which I missed.
A further contributing factor for my misunderstading was the slightly
convoluted logic where `is_clean` is set to true if there are _not_ any
uncommitted changes, and then we ask if `is_clean` is _not_ true. Reminds
me of Smullyan's Knights & Knaves [*1*].
With your patch, there are now four users of the `is_clean` value, and
all but one of them ask for the negated value.
It's not really the responsibility of this patch series, but I could
imagine that it would be nicer to future readers if a patch was added that
would invert the meaning of that variable and rename it to
`needs_committing`. At least to me, that would make the intention of the
code eminently clearer.
Ciao,
Johannes
Footnote *1*: https://en.wikipedia.org/wiki/Knights_and_Knaves
^ permalink raw reply
* Re: [PATCH v2 06/10] parse-options: mark unused "opt" parameter in callbacks
From: Jeff King @ 2023-09-05 7:05 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano
In-Reply-To: <98d1cd21-fb2a-269a-8d0b-f3e050682739@web.de>
On Sat, Sep 02, 2023 at 12:12:56PM +0200, René Scharfe wrote:
> Am 31.08.23 um 23:21 schrieb Jeff King:
> > The previous commit argued that parse-options callbacks should try to
> > use opt->value rather than touching globals directly. In some cases,
> > however, that's awkward to do. Some callbacks touch multiple variables,
> > or may even just call into an abstracted function that does so.
> >
> > In some of these cases we _could_ convert them by stuffing the multiple
> > variables into a single struct and passing the struct pointer through
> > opt->value. But that may make other parts of the code less readable,
> > as the struct relationship has to be mentioned everywhere.
>
> Does that imply you'd be willing to use other methods? Let's find out
> below. :)
Well, I'm not necessarily _opposed_. :) Mostly my cutoff was cases where
the end result was not obviously and immediately more readable. It is
not that big a deal to add an UNUSED annotation. My main goal was to use
the opportunity to check that we aren't papering over an obvious bug.
So for example...
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index 369bd43fb2..b842349d86 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)
> > strbuf_release(&config_name);
> > }
> >
> > -static int task_option_parse(const struct option *opt,
> > +static int task_option_parse(const struct option *opt UNUSED,
>
> Only the global variable "tasks" seems to be used in here if you don't
> count the constant "TASK__COUNT", so you could pass it in. This could
> also be converted to OPT_STRING_LIST with parsing and duplicate checking
> done later.
...in many cases things can be simplified by parsing into a string, and
then validating or acting on the string later. And sometimes that's even
a better strategy (because it lets the arg parsing handle all the "last
one wins" logic and we just get the result).
But it can also make the code harder to follow, too, because it's now
split it into segments (although one is mostly declarative, which is
nice).
So I generally tried to err on the side of not touching working
code. Both to avoid breaking it, but also to keep the focus on the goal
of the series. And I think that applies to a lot of the other cases you
mentioned below (I won't respond to each; I think some of them could be
fine, but it also feels like writing and review effort for not much
gain. I'm not opposed if you want to dig into them, though).
> And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION)
> on error, but that's a different matter.
Yeah, that doesn't make sense at all.
> > -static int clear_decorations_callback(const struct option *opt,
> > - const char *arg, int unset)
> > +static int clear_decorations_callback(const struct option *opt UNUSED,
> > + const char *arg, int unset)
> > {
> > string_list_clear(&decorate_refs_include, 0);
> > string_list_clear(&decorate_refs_exclude, 0);
> > use_default_decoration_filter = 0;
> > return 0;
> > }
> >
>
> Meta: Why do we get seven lines of context in an -U3 patch here? Did
> you use --inter-hunk-context?
Yes, I set diff.interhunkcontext=1 in my config (and have for many
years; I think this is the first time anybody noticed in a patch). My
rationale is that with "1" the output is never longer, since you save
the hunk header. It is a little funny in this case, since the two
changes aren't really connected.
> This patch would be better viewed with --function-context to see that
> the callbacks change multiple variables or do other funky things. Only
> doubles the line count.
I do sometimes use options like that to make a patch more readable, if I
notice the difference. I didn't happen to in this case (though I don't
disagree with you). As a reviewer, I typically apply and run "git show"
myself if I want to dig more (or just go directly look at the preimage
in my local repo, of course).
> > -static int option_parse_strategy(const struct option *opt,
> > +static int option_parse_strategy(const struct option *opt UNUSED,
>
> Could be an OPT_STRING_LIST and parsing done later. Except that
> --no-strategy does nothing, which is weird.
Yeah, I think the handling of "unset" is a bug (similar to the xopts one
fixed earlier).
-Peff
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Tao Klerks @ 2023-09-05 15:48 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: Johannes Schindelin, Taylor Blau, Patrick Steinhardt, git
In-Reply-To: <b5833396-7e04-465f-96f6-69d5280fa023@app.fastmail.com>
On Mon, Sep 4, 2023 at 7:57 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
>
> And then from that vantage point it might feel wasteful to dedicate an
> unused main worktree—with its own working tree—to just sit somewhere for
> its `.git` directory, essentially.
Yep, especially in my case where a worktree contains 200,000 files and
5GB; especially on Windows, that's a high tax to pay pointlessly.
>
> The glossary says under “worktree” (on Git 2.42):
>
> > A repository can have zero (i.e. bare repository) or one or more
> > worktrees attached to it.
>
> And as someone who never has needed to use a bare repository + worktrees
> I've just left it at that.
>
Thank you for this reference - given Eric's later answers I'll aim to
change the text rather than respect its current implication, but
knowing what needs to change is hugely helpful :)
(not that I know what to change the text *to*, but I guess there's
time to think about that)
^ permalink raw reply
* Re: [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks
From: Jeff King @ 2023-09-05 6:52 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano
In-Reply-To: <8f03fa13-11b3-1f9f-1e1f-8f7d8ce74a23@web.de>
On Sat, Sep 02, 2023 at 09:34:32AM +0200, René Scharfe wrote:
> > static int option_parse_if_missing(const struct option *opt,
> > const char *arg, int unset)
> > {
> > - return trailer_set_if_missing(&if_missing, arg);
> > + return trailer_set_if_missing(opt->value, arg);
> > }
>
> Not your fault, but these all silently exit if "arg" contains an
> unrecognized value. Reporting the error would be better.
Hmm, yeah. On the config side, git_trailer_default_config() issues a
warning after the functions return -1. I'd have guessed we at least
printed a usage message or something here, but we don't even do that.
You get a silent 129 exit code.
Gross, but it's something I think we should fix separately, as it's
orthogonal to this series.
> > static void new_trailers_clear(struct list_head *trailers)
> > @@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
> > OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
> > OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
> >
> > - OPT_CALLBACK(0, "where", NULL, N_("action"),
> > + OPT_CALLBACK(0, "where", &where, N_("action"),
> > N_("where to place the new trailer"), option_parse_where),
> > - OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
> > + OPT_CALLBACK(0, "if-exists", &if_exists, N_("action"),
> > N_("action if trailer already exists"), option_parse_if_exists),
> > - OPT_CALLBACK(0, "if-missing", NULL, N_("action"),
> > + OPT_CALLBACK(0, "if-missing", &if_missing, N_("action"),
> > N_("action if trailer is missing"), option_parse_if_missing),
>
> And I wonder if "action" should be replaced by "(after|before|end|start)",
> "(addIfDifferent|addIfDifferentNeighbor|add|replace|doNothing)" and
> "(doNothing|add)", respectively. Gets a bit long in the middle, but would
> be more helpful. #leftoverbits
I don't have a strong opinion. It is sometimes nice to provide more
detail to save the user having to look it up separately. But sometimes
those details can be overwhelming and make it hard to read, especially
if they grow over time. I'm thinking less of "-h" output and more of
people like to put:
git foo [--optionA|--optionB|--optionC]
and so on, until eventually the options block is like 4 lines long. Just
saying [options] and then listing them is more friendly. I'm not sure if
we're approaching that kind of problem here or not.
-Peff
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Eric Sunshine @ 2023-09-05 0:38 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: Tao Klerks, Johannes Schindelin, Taylor Blau, Patrick Steinhardt,
git
In-Reply-To: <b5833396-7e04-465f-96f6-69d5280fa023@app.fastmail.com>
On Mon, Sep 4, 2023 at 1:57 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
> On Mon, Sep 4, 2023, at 16:41, Tao Klerks wrote:
> > Because worktree use was so useful/widespread/critical on this project,
> > and we already had a custom cloning process that introduced selective
> > refspecs etc, we introduced a special clone topology: the initial clone
> > is a bare repo, and that folder gets a specific clear name (ending in
> > .git). Then we create worktrees attached to that bare repo.
>
> This is interesting as a Git user. I've been encountering questions on
> StackOverflow where the questioner is using a bare repository which they
> make (or try to make) worktrees from. I've been telling them that making
> worktrees from a bare repository is a contradiction:[1]
Not at all. The combination of bare repository and multiple-worktrees
is legitimate and supported intentionally. (There are tests in the Git
test suite validating support of this feature.) For people who
regularly work with multiple worktrees, it is quite natural to have
all the worktrees hanging off a bare repository, each with equal
importance, rather than having a single "blessed" worktree which has
priority over all others.
> > Bare repositories don’t have worktrees per definition. Or at least
> > that’s what `man gitglossary says`. Of course what `git worktree` allows
> > you to do trumps that. But it might be ill-defined.
>
> The glossary says under “worktree” (on Git 2.42):
>
> > A repository can have zero (i.e. bare repository) or one or more
> > worktrees attached to it.
Speaking as a person involved in the implementation of worktrees,
including support for them in combination with bare repositories, my
reading of this is perhaps biased so that I understand its intent.
However, if I squint hard, I suppose I can see how you could read it
as meaning that a bare repository can't have any worktrees associated
with it. So, perhaps, the documentation could use a bit of touch up.
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Junio C Hamano @ 2023-09-05 1:09 UTC (permalink / raw)
To: Eric Sunshine
Cc: Tao Klerks, git, Johannes Schindelin, Taylor Blau,
Patrick Steinhardt
In-Reply-To: <CAPig+cTeQDMpWQ-zCf6i9H-yhrdCndX6gs67sypuqmHZZcHm7w@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> This is not accurate. There is no default location for new worktrees;
> git-worktree creates the new worktree at the location specified by the
> user:
>
> git worktree add [<options>] <path> [<commit>]
>
> where <path> -- the only mandatory argument -- specifies the location.
All correct. The per-worktree part of the repository data does live
in a subdirectory of the ".git" directory and that was probably what
Tao had in mind, though.
> It indeed was designed to work this way. It is perfectly legitimate to
> create worktrees attached to a bare repository[1].
>
> [1]: Support for bare repositories in conjunction with multiple-
> worktrees, however, came after the initial implementation of multiple-
> worktrees. An unfortunate side-effect is that established terminology
> became somewhat confusing. In particular, in a bare repository
> scenario, the term "main worktree" refers to the bare repository, not
> to the "blessed" worktree containing the ".git/" directory (since
> there is no such worktree in this case).
Again all correct.
>> Is it the case that this contrib script predates the current "git
>> worktree" support?
>
> git-new-workdir predates git-worktree by quite a few years and, as I
> understand it, remains in-tree because it fills a niche not entirely
> filled by git-worktree.
I actually think there is no longer a valid workflow whose support
by "worktree" is still insufficient and the script has outlived its
usefulness. I have been a heavy user of the new-workdir script to
maintain my build environments, but I always have the HEAD of these
workdir's detached, so I can easily switch my arrangement to use the
"git worktree" without losing any flexibility.
Perhaps we should remove it, possibly leaving a tombstone file like
how we removed stuff from the contrib/examples directory.
^ permalink raw reply
* Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth
From: Jeff King @ 2023-09-05 7:21 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <4d2eb736-4f34-18f8-2eb7-20e7f7b8c2f8@web.de>
On Sat, Sep 02, 2023 at 08:54:54PM +0200, René Scharfe wrote:
> a91f453f64 (grep: Add --max-depth option., 2009-07-22) added the option
> --max-depth, defining it using a positional struct option initializer of
> type OPTION_INTEGER. It also sets defval to 1 for some reason, but that
> value would only be used if the flag PARSE_OPT_OPTARG was given.
>
> Use the macro OPT_INTEGER_F instead to standardize the definition and
> specify only the necessary values. This also normalizes argh to N_("n")
> as a side-effect, which is OK.
This looks correct to me (and an improvement in readability). In
general, I wonder how many of the results from:
git grep '{ OPTION'
could be converted to use the macros and end up more readable. There are
a number of OPTARG ones, which I guess can't use macros. Looks like
there are a handful of others (mostly for OPT_HIDDEN).
-Peff
^ permalink raw reply
* Re: [PATCH v3 6/7] rebase --continue: refuse to commit after failed command
From: Junio C Hamano @ 2023-09-05 14:57 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Phillip Wood, Phillip Wood via GitGitGadget, git, Stefan Haller,
Eric Sunshine, Glen Choo
In-Reply-To: <87fbc8c9-f42b-b374-fee1-57c58f5e8fc0@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> With your patch, there are now four users of the `is_clean` value, and
> all but one of them ask for the negated value.
Excellent observation. That strongly argues for the flipping of
polarity, i.e. many people want to know "is it unclean/dirty?". It
is funny that the name of the helper function where the value comes
from, i.e. has_uncommitted_changes(), has the desired polarity.
> It's not really the responsibility of this patch series, but I could
> imagine that it would be nicer to future readers if a patch was added that
> would invert the meaning of that variable and rename it to
> `needs_committing`. At least to me, that would make the intention of the
> code eminently clearer.
While I agree, after reading the code, that it would make it easier
to follow to flip the polarity of the variable, I would advise
against renaming from state based naming (is it dirty?) to action
based naming (must we commit?), *if* the variable is checked to
sometimes see if we has something that we _could_ commit, while some
other times to see if we _must_ commit before we can let the user
proceed.
"Does the index hold some changes to be committed?" is better
question than "Must we commit?" or "Could we commit?" to derive the
name of this variable from if that is the case, I would think.
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Junio C Hamano @ 2023-09-05 15:13 UTC (permalink / raw)
To: Eric Sunshine
Cc: Tao Klerks, git, Johannes Schindelin, Taylor Blau,
Patrick Steinhardt
In-Reply-To: <CAPig+cRJhrGmnBRm2dporcXiRr4SzRmpM2LTMm0S7wo0XbOU9Q@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
>> All correct. The per-worktree part of the repository data does live
>> in a subdirectory of the ".git" directory and that was probably what
>> Tao had in mind, though.
>
> That could be. I read Tao's explanation as meaning that people do this:
>
> git clone foo.git foo
> cd foo
> git worktree add bar
> git worktree add baz
>
> rather than (perhaps) this:
>
> git clone foo.git foo
> cd foo
> git worktree add ../bar
> git worktree add ../baz
Ah, that reading does totally make sense.
But I am not sure it would lead to "we need to carefully protect the
primary worktree", because it is rather obvious, especially if you
bypass "git worktree remove" and use "rm -fr", you would lose
everybody underneath if you remove the "foo" in the "worktrees are
subdirectories of the primary" variant in the above examples.
Even though deriving the worktree(s) from a separate and protected
bare repositories does protect you from total disaster caused by
removing "rm -fr" and bypassing "git worktree remove", it still
should be discouraged, as the per-worktree states left behind in the
repository interfere with the operations in surviving worktrees.
Teaching folks not to do "rm -fr" would be the first step to a more
pleasant end-user experience, I would think.
Thanks.
^ permalink raw reply
* [PATCH v3 04/10] checkout-index: delay automatic setting of to_tempfile
From: Jeff King @ 2023-09-05 7:12 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano
In-Reply-To: <c7855b08-46ee-5df0-4b0f-67ea57d84b18@web.de>
On Sat, Sep 02, 2023 at 08:20:43AM +0200, René Scharfe wrote:
> Am 31.08.23 um 23:20 schrieb Jeff King:
> > @@ -269,6 +268,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> > state.base_dir = "";
> > state.base_dir_len = strlen(state.base_dir);
> >
> > + if (to_tempfile < 0)
> > + to_tempfile = (checkout_stage == CHECKOUT_ALL);
> > + if (!to_tempfile && checkout_stage == CHECKOUT_ALL)
> > + die("--stage=all and --no-temp are incompatible");
>
> How about making this message translatable from the start and following
> the convention from 12909b6b8a (i18n: turn "options are incompatible"
> into "cannot be used together", 2022-01-05) to reuse the existing
> translations?
Good catch. I forgot that we had standardized some of these. The other
error messages in the file aren't translated, but I don't think that's
an intentional choice (even though it is plumbing). Some of them are
obviously quite old and don't match our usual style (like starting with
"checkout-index: ").
Rather than re-send the whole series, I _think_ this is the only patch I
would change in a re-roll (if you buy me "sure, go ahead and send it on
top" evasions in my other responses).
So here's a replacement patch 4 that fixes up the message.
-- >8 --
Subject: checkout-index: delay automatic setting of to_tempfile
Using --stage=all requires writing to tempfiles, since we cannot put
multiple stages into a single file. So --stage=all implies --temp.
But we do so by setting to_tempfile in the options callback for --stage,
rather than after all options have been parsed. This leads to two bugs:
1. If you run "checkout-index --stage=all --stage=2", this should not
imply --temp, but it currently does. The callback cannot just unset
to_tempfile when it sees the "2" value, because it no longer knows
if its value was from the earlier --stage call, or if the user
specified --temp explicitly.
2. If you run "checkout-index --stage=all --no-temp", the --no-temp
will overwrite the earlier implied --temp. But this mode of
operation cannot work, and the command will fail with "<path>
already exists" when trying to write the higher stages.
We can fix both by lazily setting to_tempfile. We'll make it a tristate,
with -1 as "not yet given", and have --stage=all enable it only after
all options are parsed. Likewise, after all options are parsed we can
detect and reject the bogus "--no-temp" case.
Note that this does technically change the behavior for "--stage=all
--no-temp" for paths which have only one stage present (which
accidentally worked before, but is now forbidden). But this behavior was
never intended, and you'd have to go out of your way to try to trigger
it.
The new tests cover both cases, as well the general "--stage=all implies
--temp", as most of the other tests explicitly say "--temp". Ironically,
the test "checkout --temp within subdir" is the only one that _doesn't_
use "--temp", and so was implicitly covering this case. But it seems
reasonable to have a more explicit test alongside the other related
ones.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/checkout-index.c | 9 +++++++--
t/t2004-checkout-cache-temp.sh | 20 ++++++++++++++++++++
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index f62f13f2b5..526c210bcb 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -24,7 +24,7 @@
static int nul_term_line;
static int checkout_stage; /* default to checkout stage0 */
static int ignore_skip_worktree; /* default to 0 */
-static int to_tempfile;
+static int to_tempfile = -1;
static char topath[4][TEMPORARY_FILENAME_LENGTH + 1];
static struct checkout state = CHECKOUT_INIT;
@@ -196,7 +196,6 @@ static int option_parse_stage(const struct option *opt,
BUG_ON_OPT_NEG(unset);
if (!strcmp(arg, "all")) {
- to_tempfile = 1;
checkout_stage = CHECKOUT_ALL;
} else {
int ch = arg[0];
@@ -269,6 +268,12 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
state.base_dir = "";
state.base_dir_len = strlen(state.base_dir);
+ if (to_tempfile < 0)
+ to_tempfile = (checkout_stage == CHECKOUT_ALL);
+ if (!to_tempfile && checkout_stage == CHECKOUT_ALL)
+ die(_("options '%s' and '%s' cannot be used together"),
+ "--stage=all", "--no-temp");
+
/*
* when --prefix is specified we do not want to update cache.
*/
diff --git a/t/t2004-checkout-cache-temp.sh b/t/t2004-checkout-cache-temp.sh
index b16d69ca4a..45dd1bc858 100755
--- a/t/t2004-checkout-cache-temp.sh
+++ b/t/t2004-checkout-cache-temp.sh
@@ -117,6 +117,26 @@ test_expect_success 'checkout all stages/one file to temporary files' '
test $(cat $s3) = tree3path1)
'
+test_expect_success '--stage=all implies --temp' '
+ rm -f path* .merge_* actual &&
+ git checkout-index --stage=all -- path1 &&
+ test_path_is_missing path1
+'
+
+test_expect_success 'overriding --stage=all resets implied --temp' '
+ rm -f path* .merge_* actual &&
+ git checkout-index --stage=all --stage=2 -- path1 &&
+ echo tree2path1 >expect &&
+ test_cmp expect path1
+'
+
+test_expect_success '--stage=all --no-temp is rejected' '
+ rm -f path* .merge_* actual &&
+ test_must_fail git checkout-index --stage=all --no-temp -- path1 2>err &&
+ grep -v "already exists" err &&
+ grep "options .--stage=all. and .--no-temp. cannot be used together" err
+'
+
test_expect_success 'checkout some stages/one file to temporary files' '
rm -f path* .merge_* actual &&
git checkout-index --stage=all --temp -- path2 >actual &&
--
2.42.0.567.gc396a4a104
^ 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