* Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge
From: Patrick Steinhardt @ 2024-01-15 11:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Lohmann, git, newren, phillip.wood123
In-Reply-To: <xmqqzfxa9usx.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]
On Fri, Jan 12, 2024 at 12:10:54PM -0800, Junio C Hamano wrote:
> Michael Lohmann <mi.al.lohmann@gmail.com> writes:
>
> >> but we may want to tighten the original's use of repo_get
> >> > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> >> > - die("--merge without MERGE_HEAD?");
> >> > - other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> >>
> >> ... so that we won't be confused in a repository that has a tag
> >> whose name happens to be MERGE_HEAD. We probably should be using
> >> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...
> >
> > Here I am really at the limit of my understanding of the core functions.
> > Is this roughly what you had in mind? From my testing it seems to do the
> > job, but I don't understand the details "refs_resolve_ref_unsafe"...
>
> Perhaps there are others who are more familiar with the ref API than
> I am, but I was just looking at refs.h today and wonder if something
> along the lines of this ...
>
> if (read_ref_full("MERGE_HEAD",
> RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> &oid, NULL))
> die("no MERGE_HEAD");
> if (is_null_oid(&oid))
> die("MERGE_HEAD is a symbolic ref???");
>
> ... would be simpler.
>
> With the above, we are discarding the refname read_ref_full()
> obtains from its refs_resolve_ref_unsafe(), but I think that is
> totally fine. We expect it to be "MERGE_HEAD" in the good case.
> The returned value can be different from "MERGE_HEAD" if it is
> a symbolic ref, but if the comment in refs.h on NO_RECURSE is to be
> trusted, we should catch that case with the NULL-ness check on oid.
Yeah, this should be fine.
Even though I have stared at our refs API for extended periods of time
during the last months I still have to look up this part of the API
almost every time. I find it to be hard to use because you not only have
to pay attention to the return value, but also to the flags in some edge
cases. I wouldn't be surprised if there were many callsites that get
this wrong.
> Which would mean that we do not need a separate "other_head"
> variable, and instead can use "MERGE_HEAD".
There is no need for this, is there? We have already resolved the ref to
an object ID, so why not use that via `add_pending_oid()`?
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-15 11:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Taylor Blau, Jeff King, Dragan Simic
In-Reply-To: <xmqqsf326vpn.fsf@gitster.g>
On 12-ene-2024 14:19:32, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>
> > advice.c | 109 +++++++++++++++++++++++++---------------------
> > t/t0018-advice.sh | 1 -
> > 2 files changed, 59 insertions(+), 51 deletions(-)
> >
> > diff --git a/advice.c b/advice.c
> > index f6e4c2f302..8474966ce1 100644
> > --- a/advice.c
> > +++ b/advice.c
> > @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)
> > return "";
> > }
> >
> > +enum advice_level {
> > + ADVICE_LEVEL_DEFAULT = 0,
>
> We may want to say "_NONE" not "_DEFAULT" to match the convention
> used elsewhere, but I have no strong preference.
The "_DEFAULT" name is rooted in the idea of having a default but
configurable value. I'm not developing that idea in this series because
I'm not sure if it's desirable. But, I'll leave a sketch here:
diff --git a/advice.c b/advice.c
index 8474966ce1..1bb427a8d8 100644
--- a/advice.c
+++ b/advice.c
@@ -39,6 +39,8 @@ enum advice_level {
ADVICE_LEVEL_ENABLED,
};
+static enum advice_level advice_level_default;
+
static struct {
const char *key;
enum advice_level level;
@@ -122,7 +124,9 @@ void advise(const char *advice, ...)
int advice_enabled(enum advice_type type)
{
- int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+ int enabled = advice_setting[type].level
+ ? advice_setting[type].level != ADVICE_LEVEL_DISABLED
+ : advice_level_default != ADVICE_LEVEL_DISABLED;
if (type == ADVICE_PUSH_UPDATE_REJECTED)
return enabled &&
@@ -139,7 +143,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
return;
va_start(params, advice);
- vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
+ vadvise(advice, !advice_setting[type].level && !advice_level_default,
advice_setting[type].key, params);
va_end(params);
}
@@ -166,6 +170,13 @@ int git_default_advice_config(const char *var, const char *value)
if (!skip_prefix(var, "advice.", &k))
return 0;
+ if (!strcmp(var, "advice.default")) {
+ advice_level_default = git_config_bool(var, value)
+ ? ADVICE_LEVEL_ENABLED
+ : ADVICE_LEVEL_DISABLED;
+ return 0;
+ }
+
for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
if (strcasecmp(k, advice_setting[i].key))
continue;
The way it is now, "_NONE" is perfectly fine. I'll use it.
> I do have a
> preference to see that, when we explicitly say "In this enum, there
> is ADVICE_LEVEL_DEFAULT and its value is zero" with "= 0" in the
> definition, the places we use a variable of this enum type, we say
>
> if (!variable)
OK
>
> and not
>
> if (variable == ADVICE_LEVEL_DEFAULT)
>
> > + ADVICE_LEVEL_DISABLED,
> > + ADVICE_LEVEL_ENABLED,
> > +};
> > +
> > static struct {
> > const char *key;
> > - int enabled;
> > + enum advice_level level;
> > } advice_setting[] = {
> > ...
> > - [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 },
> > + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo" },
> > ...
> > + [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan" },
> > };
>
> It certainly becomes nicer to use the "unspecified is left to 0"
> convention like this.
Indeed, that's my feeling too.
>
> > static const char turn_off_instructions[] =
> > @@ -116,13 +122,13 @@ void advise(const char *advice, ...)
> >
> > int advice_enabled(enum advice_type type)
> > {
> > - switch(type) {
> > - case ADVICE_PUSH_UPDATE_REJECTED:
> > - return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
> > - advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;
> > - default:
> > - return advice_setting[type].enabled;
> > - }
> > + int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
>
> OK.
>
> > + if (type == ADVICE_PUSH_UPDATE_REJECTED)
> > + return enabled &&
> > + advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
>
> I like the textbook use of a simple recursion here ;-)
>
> > + return enabled;
> > }
>
> > void advise_if_enabled(enum advice_type type, const char *advice, ...)
> > @@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
> > return;
> >
> > va_start(params, advice);
> > - vadvise(advice, 1, advice_setting[type].key, params);
> > + vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
> > + advice_setting[type].key, params);
>
> OK. Once you configure this advice to be always shown, you no
> longer are using the _DEFAULT, so we pass 0 as the second parameter.
> Makes sense.
>
> As I said, if we explicitly document that _DEFAULT's value is zero
> with "= 0" in the enum definition, which we also rely on the
> initialization of the advice_setting[] array, then it may probably
> be better to write it more like so:
>
> vadvice(advice, !advice_setting[type].level,
> advice_setting[type].key, params);
OK
>
> I wonder if it makes this caller simpler to update the return value
> of advice_enabled() to a tristate. Then we can do
>
> int enabled = advice_enabled(type);
>
> if (!enabled)
> return;
> va_start(...);
> vadvice(advice, enabled != ADVICE_LEVEL_ENABLED, ...);
> va_end(...);
>
> but it does not make that much difference.
>
> > va_end(params);
> > }
> >
> > @@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)
> > for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
> > if (strcasecmp(k, advice_setting[i].key))
> > continue;
> > - advice_setting[i].enabled = git_config_bool(var, value);
> > + advice_setting[i].level = git_config_bool(var, value)
> > + ? ADVICE_LEVEL_ENABLED
> > + : ADVICE_LEVEL_DISABLED;
>
> OK. We saw (var, value) in the configuration files we are parsing,
> so we find [i] that corresponds to the advice key and tweak the
> level.
>
> This loop obviously is O(N*M) for the number of "advice.*"
> configuration variables N and the number of advice keys M, but that
> is not new to this patch---our expectation is that N will be small,
> even though M will grow over time.
I wonder if we can drop entirely this loop. Maybe a hashmap can be
helpful here. Of course, this is tangential to this series.
>
> > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> > index c13057a4ca..0dcfb760a2 100755
> > --- a/t/t0018-advice.sh
> > +++ b/t/t0018-advice.sh
> > @@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '
> > test_expect_success 'advice should be printed when config variable is set to true' '
> > cat >expect <<-\EOF &&
> > hint: This is a piece of advice
> > - hint: Disable this message with "git config advice.nestedTag false"
> > EOF
> > test_config advice.nestedTag true &&
> > test-tool advise "This is a piece of advice" 2>actual &&
>
> OK. The commit changes behaviour for existing users who explicitly
> said "I want to see this advice" by configuring advice.* key to 'true',
Technically, when the user sets any value.
Maybe in the future we transform the knob to have more than two states,
beyond yes/no. At that point, current rationality would still apply.
> and it probably is a good thing, even though it may be a bit surprising.
>
> One thing that may be missing is a documentation update. Something
> along this line to highlight that now 'true' has a bit different
> meaning from before (and as a consequence, we can no longer say
> "defaults to true").
>
> Documentation/config/advice.txt | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git c/Documentation/config/advice.txt w/Documentation/config/advice.txt
> index 2737381a11..364a8268b6 100644
> --- c/Documentation/config/advice.txt
> +++ w/Documentation/config/advice.txt
> @@ -1,7 +1,11 @@
> advice.*::
> - These variables control various optional help messages designed to
> - aid new users. All 'advice.*' variables default to 'true', and you
> - can tell Git that you do not need help by setting these to 'false':
> +
> + These variables control various optional help messages
> + designed to aid new users. When left unconfigured, Git will
> + give you the help message with an instruction on how to
> + squelch it. When set to 'true', the help message is given,
> + but without hint on how to squelch the message. You can
> + tell Git that you do not need help by setting these to 'false':
> +
> --
> ambiguousFetchRefspec::
OK. I'll add this.
Thanks.
^ permalink raw reply related
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Phillip Wood @ 2024-01-15 10:48 UTC (permalink / raw)
To: Michael Lohmann; +Cc: git, phillip.wood, Junio C Hamano
In-Reply-To: <20240112150346.73735-1-mi.al.lohmann@gmail.com>
Hi Michael
On 12/01/2024 15:03, Michael Lohmann wrote:
> Hi Phillip,
>
> On 12. Jan 2024, at 12:01, phillip.wood123@gmail.com wrote:
>> I should start by saying that I didn't know "git log --merge" existed before
>> I saw this message
> I also just found it and it looked very useful...
>
>> so please correct me if I've misunderstood what this patch is doing. If I
>> understand correctly it shows the commits from each side of the merge and is
>> equivalent to
>>
>> git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)
>>
>> When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ [*]
>> so I'm not sure what
>>
>> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)
>
> Almost, but not quite: "git log —merge" only shows the commits touching the
> conflict, so it would be equivalent to (I think):
>
> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative)
>
> (or replace CHERRY_PICK with one of the other actions)
Thanks for clarifying that.
>> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor.
>
> True - but same for MERGE_HEAD ("git merge --allow-unrelated-histories"). I
> have to confess I did not check how it would behave under those circumstances.
> It could either error, or (more helpful) show the log touching the file until
> the root commit.
What I was trying to get at was that with "git merge" "git log --merge"
will show commits that are part of the merge. With "git cherry-pick"
that's not the case because we're selecting the commits to show using
the merge base of HEAD and CHERRY_PICK_HEAD while cherry-pick uses
CHERRY_PICK_HEAD^ as the base of the merge. I think Junio explains why
it is still useful to show those commits in [1] i.e. they help the user
understand the conflicts even though they are not part of the merge. It
might be worth expanding the commit message to explain that.
Best Wishes
Phillip
[1] https://lore.kernel.org/git/xmqqil3y9rvm.fsf@gitster.g/
^ permalink raw reply
* Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Phillip Wood @ 2024-01-15 10:39 UTC (permalink / raw)
To: Achu Luma, git
Cc: chriscool, christian.couder, gitster, l.s.r, me, phillip.wood,
steadmon
In-Reply-To: <20240112102743.1440-1-ach.lumap@gmail.com>
Hi Achu
On 12/01/2024 10:27, Achu Luma wrote:
> In the recent codebase update (8bf6fbd00d (Merge branch
> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> merged, providing a standardized approach for testing C code. Prior to
> this update, some unit tests relied on the test helper mechanism,
> lacking a dedicated unit testing framework. It's more natural to perform
> these unit tests using the new unit test framework.
>
> This commit migrates the unit tests for C character classification
> functions (isdigit(), isspace(), etc) from the legacy approach
> using the test-tool command `test-tool ctype` in t/helper/test-ctype.c
> to the new unit testing framework (t/unit-tests/test-lib.h).
>
> The migration involves refactoring the tests to utilize the testing
> macros provided by the framework (TEST() and check_*()).
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Helped-by: René Scharfe <l.s.r@web.de>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Helped-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Achu Luma <ach.lumap@gmail.com>
> ---
> The change between version 4 and version 5 is:
> - Added tests to handle EOF.
>
> Thanks to Phillip for noticing the missing tests..
> Here is a diff between v4 and v5:
>
> + if (!check(!func(EOF))) \
> + test_msg(" i: 0x%02x (EOF)", EOF); \
Thanks for adding back the test for EOF, this version looks good to me.
Best Wishes
Phillip
> Thanks also to René, Phillip, Junio and Taylor who helped with
> previous versions.
>
> Makefile | 2 +-
> t/helper/test-ctype.c | 70 ------------------------------------
> t/helper/test-tool.c | 1 -
> t/helper/test-tool.h | 1 -
> t/t0070-fundamental.sh | 4 ---
> t/unit-tests/t-ctype.c | 80 ++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 81 insertions(+), 77 deletions(-)
> delete mode 100644 t/helper/test-ctype.c
> create mode 100644 t/unit-tests/t-ctype.c
>
> diff --git a/Makefile b/Makefile
> index 15990ff312..1a62e48759 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o
> TEST_BUILTINS_OBJS += test-config.o
> TEST_BUILTINS_OBJS += test-crontab.o
> TEST_BUILTINS_OBJS += test-csprng.o
> -TEST_BUILTINS_OBJS += test-ctype.o
> TEST_BUILTINS_OBJS += test-date.o
> TEST_BUILTINS_OBJS += test-delta.o
> TEST_BUILTINS_OBJS += test-dir-iterator.o
> @@ -1342,6 +1341,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
> UNIT_TEST_PROGRAMS += t-basic
> UNIT_TEST_PROGRAMS += t-mem-pool
> UNIT_TEST_PROGRAMS += t-strbuf
> +UNIT_TEST_PROGRAMS += t-ctype
> UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
> UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
> UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
> diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
> deleted file mode 100644
> index e5659df40b..0000000000
> --- a/t/helper/test-ctype.c
> +++ /dev/null
> @@ -1,70 +0,0 @@
> -#include "test-tool.h"
> -
> -static int rc;
> -
> -static void report_error(const char *class, int ch)
> -{
> - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
> - rc = 1;
> -}
> -
> -static int is_in(const char *s, int ch)
> -{
> - /*
> - * We can't find NUL using strchr. Accept it as the first
> - * character in the spec -- there are no empty classes.
> - */
> - if (ch == '\0')
> - return ch == *s;
> - if (*s == '\0')
> - s++;
> - return !!strchr(s, ch);
> -}
> -
> -#define TEST_CLASS(t,s) { \
> - int i; \
> - for (i = 0; i < 256; i++) { \
> - if (is_in(s, i) != t(i)) \
> - report_error(#t, i); \
> - } \
> - if (t(EOF)) \
> - report_error(#t, EOF); \
> -}
> -
> -#define DIGIT "0123456789"
> -#define LOWER "abcdefghijklmnopqrstuvwxyz"
> -#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> -#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
> -#define ASCII \
> - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> - "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
> - "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
> - "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
> - "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
> - "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
> - "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
> -#define CNTRL \
> - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> - "\x7f"
> -
> -int cmd__ctype(int argc UNUSED, const char **argv UNUSED)
> -{
> - TEST_CLASS(isdigit, DIGIT);
> - TEST_CLASS(isspace, " \n\r\t");
> - TEST_CLASS(isalpha, LOWER UPPER);
> - TEST_CLASS(isalnum, LOWER UPPER DIGIT);
> - TEST_CLASS(is_glob_special, "*?[\\");
> - TEST_CLASS(is_regex_special, "$()*+.?[\\^{|");
> - TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
> - TEST_CLASS(isascii, ASCII);
> - TEST_CLASS(islower, LOWER);
> - TEST_CLASS(isupper, UPPER);
> - TEST_CLASS(iscntrl, CNTRL);
> - TEST_CLASS(ispunct, PUNCT);
> - TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF");
> - TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
> -
> - return rc;
> -}
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 37ba996539..33b9501c21 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {
> { "config", cmd__config },
> { "crontab", cmd__crontab },
> { "csprng", cmd__csprng },
> - { "ctype", cmd__ctype },
> { "date", cmd__date },
> { "delta", cmd__delta },
> { "dir-iterator", cmd__dir_iterator },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 8a1a7c63da..b72f07ded9 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);
> int cmd__config(int argc, const char **argv);
> int cmd__crontab(int argc, const char **argv);
> int cmd__csprng(int argc, const char **argv);
> -int cmd__ctype(int argc, const char **argv);
> int cmd__date(int argc, const char **argv);
> int cmd__delta(int argc, const char **argv);
> int cmd__dir_iterator(int argc, const char **argv);
> diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
> index 487bc8d905..a4756fbab9 100755
> --- a/t/t0070-fundamental.sh
> +++ b/t/t0070-fundamental.sh
> @@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.
> TEST_PASSES_SANITIZE_LEAK=true
> . ./test-lib.sh
>
> -test_expect_success 'character classes (isspace, isalpha etc.)' '
> - test-tool ctype
> -'
> -
> test_expect_success 'mktemp to nonexistent directory prints filename' '
> test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&
> grep "doesnotexist/test" err
> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
> new file mode 100644
> index 0000000000..f315489984
> --- /dev/null
> +++ b/t/unit-tests/t-ctype.c
> @@ -0,0 +1,80 @@
> +#include "test-lib.h"
> +
> +static int is_in(const char *s, int ch)
> +{
> + /*
> + * We can't find NUL using strchr. Accept it as the first
> + * character in the spec -- there are no empty classes.
> + */
> + if (ch == '\0')
> + return ch == *s;
> + if (*s == '\0')
> + s++;
> + return !!strchr(s, ch);
> +}
> +
> +/* Macro to test a character type */
> +#define TEST_CTYPE_FUNC(func, string) \
> +static void test_ctype_##func(void) { \
> + for (int i = 0; i < 256; i++) { \
> + if (!check_int(func(i), ==, is_in(string, i))) \
> + test_msg(" i: 0x%02x", i); \
> + } \
> + if (!check(!func(EOF))) \
> + test_msg(" i: 0x%02x (EOF)", EOF); \
> +}
> +
> +#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works")
> +
> +#define DIGIT "0123456789"
> +#define LOWER "abcdefghijklmnopqrstuvwxyz"
> +#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> +#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
> +#define ASCII \
> + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> + "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
> + "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
> + "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
> + "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
> + "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
> + "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
> +#define CNTRL \
> + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> + "\x7f"
> +
> +TEST_CTYPE_FUNC(isdigit, DIGIT)
> +TEST_CTYPE_FUNC(isspace, " \n\r\t")
> +TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
> +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
> +TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
> +TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
> +TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
> +TEST_CTYPE_FUNC(isascii, ASCII)
> +TEST_CTYPE_FUNC(islower, LOWER)
> +TEST_CTYPE_FUNC(isupper, UPPER)
> +TEST_CTYPE_FUNC(iscntrl, CNTRL)
> +TEST_CTYPE_FUNC(ispunct, PUNCT)
> +TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
> +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
> +
> +int cmd_main(int argc, const char **argv) {
> + /* Run all character type tests */
> + TEST_CHAR_CLASS(isspace);
> + TEST_CHAR_CLASS(isdigit);
> + TEST_CHAR_CLASS(isalpha);
> + TEST_CHAR_CLASS(isalnum);
> + TEST_CHAR_CLASS(is_glob_special);
> + TEST_CHAR_CLASS(is_regex_special);
> + TEST_CHAR_CLASS(is_pathspec_magic);
> + TEST_CHAR_CLASS(isascii);
> + TEST_CHAR_CLASS(islower);
> + TEST_CHAR_CLASS(isupper);
> + TEST_CHAR_CLASS(iscntrl);
> + TEST_CHAR_CLASS(ispunct);
> + TEST_CHAR_CLASS(isxdigit);
> + TEST_CHAR_CLASS(isprint);
> +
> + return test_done();
> +}
> --
> 2.42.0.windows.2
>
>
^ permalink raw reply
* [PATCH v2 5/5] completion: treat dangling symrefs as existing pseudorefs
From: Patrick Steinhardt @ 2024-01-15 10:36 UTC (permalink / raw)
To: git
Cc: Stan Hu, SZEDER Gábor, Jeff King, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <cover.1705314554.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]
The `__git_pseudoref_exists ()` helper function back to git-rev-parse(1)
in case the reftable backend is in use. This is not in the same spirit
as the simple existence check that the "files" backend does though,
because there we only check for the pseudo-ref to exist with `test -f`.
With git-rev-parse(1) we not only check for existence, but also verify
that the pseudo-ref resolves to an object, which may not be the case
when the pseudo-ref points to an unborn branch.
Fix this issue by using `git show-ref --exists` instead. Note that we do
not have to silence stdout anymore as git-show-ref(1) will not print
anything.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/completion/git-completion.bash | 2 +-
t/t9902-completion.sh | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 54ce58f73d..6662db221d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -148,7 +148,7 @@ __git_pseudoref_exists ()
# platforms.
if __git_eread "$__git_repo_path/HEAD" head; then
if [ "$head" == "ref: refs/heads/.invalid" ]; then
- __git rev-parse --verify --quiet "$ref" >/dev/null
+ __git show-ref --exists "$ref"
return $?
fi
fi
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 56dc7343a2..35eb534fdd 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2743,6 +2743,10 @@ test_expect_success '__git_pseudoref_exists' '
cd repo &&
sane_unset __git_repo_path &&
+ # HEAD should exist, even if it points to an unborn branch.
+ __git_pseudoref_exists HEAD >output 2>&1 &&
+ test_must_be_empty output &&
+
# HEAD points to an existing branch, so it should exist.
test_commit A &&
__git_pseudoref_exists HEAD >output 2>&1 &&
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 4/5] completion: silence pseudoref existence check
From: Patrick Steinhardt @ 2024-01-15 10:36 UTC (permalink / raw)
To: git
Cc: Stan Hu, SZEDER Gábor, Jeff King, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <cover.1705314554.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2991 bytes --]
In 44dbb3bf29 (completion: support pseudoref existence checks for
reftables, 2023-12-19), we have extended the Bash completion script to
support future ref backends better by using git-rev-parse(1) to check
for pseudo-ref existence. This conversion has introduced a bug, because
even though we pass `--quiet` to git-rev-parse(1) it would still output
the resolved object ID of the ref in question if it exists.
Fix this by redirecting its stdout to `/dev/null` and add a test that
catches this behaviour. Note that the test passes even without the fix
for the "files" backend because we parse pseudo refs via the filesystem
directly in that case. But the test will fail with the "reftable"
backend.
Helped-by: Jeff King <peff@peff.net>
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/completion/git-completion.bash | 2 +-
t/t9902-completion.sh | 31 ++++++++++++++++++++++++++
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index d703e3e64f..54ce58f73d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -148,7 +148,7 @@ __git_pseudoref_exists ()
# platforms.
if __git_eread "$__git_repo_path/HEAD" head; then
if [ "$head" == "ref: refs/heads/.invalid" ]; then
- __git rev-parse --verify --quiet "$ref"
+ __git rev-parse --verify --quiet "$ref" >/dev/null
return $?
fi
fi
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 95ec762a74..56dc7343a2 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1933,6 +1933,14 @@ test_expect_success 'git checkout - --orphan with branch already provided comple
EOF
'
+test_expect_success 'git restore completes modified files' '
+ test_commit A a.file &&
+ echo B >a.file &&
+ test_completion "git restore a." <<-\EOF
+ a.file
+ EOF
+'
+
test_expect_success 'teardown after ref completion' '
git branch -d matching-branch &&
git tag -d matching-tag &&
@@ -2728,4 +2736,27 @@ test_expect_success '__git_complete' '
test_must_fail __git_complete ga missing
'
+test_expect_success '__git_pseudoref_exists' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ sane_unset __git_repo_path &&
+
+ # HEAD points to an existing branch, so it should exist.
+ test_commit A &&
+ __git_pseudoref_exists HEAD >output 2>&1 &&
+ test_must_be_empty output &&
+
+ # CHERRY_PICK_HEAD does not exist, so the existence check should fail.
+ ! __git_pseudoref_exists CHERRY_PICK_HEAD >output 2>&1 &&
+ test_must_be_empty output &&
+
+ # CHERRY_PICK_HEAD points to a commit, so it should exist.
+ git update-ref CHERRY_PICK_HEAD A &&
+ __git_pseudoref_exists CHERRY_PICK_HEAD >output 2>&1 &&
+ test_must_be_empty output
+ )
+'
+
test_done
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 3/5] completion: improve existence check for pseudo-refs
From: Patrick Steinhardt @ 2024-01-15 10:36 UTC (permalink / raw)
To: git
Cc: Stan Hu, SZEDER Gábor, Jeff King, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <cover.1705314554.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]
Improve the existence check along the following lines:
- Stop stripping the "ref :" prefix and compare to the expected value
directly. This allows us to drop a now-unused variable that was
previously leaking into the user's shell.
- Mark the "head" variable as local so that we don't leak its value
into the user's shell.
- Stop manually handling the `-C $__git_repo_path` option, which the
`__git ()` wrapper aleady does for us.
- In simlar spirit, stop redirecting stderr, which is also handled by
the wrapper already.
Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/completion/git-completion.bash | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 06a9107449..d703e3e64f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -137,6 +137,7 @@ __git_eread ()
__git_pseudoref_exists ()
{
local ref=$1
+ local head
__git_find_repo_path
@@ -146,9 +147,8 @@ __git_pseudoref_exists ()
# Bash builtins since executing Git commands are expensive on some
# platforms.
if __git_eread "$__git_repo_path/HEAD" head; then
- b="${head#ref: }"
- if [ "$b" == "refs/heads/.invalid" ]; then
- __git -C "$__git_repo_path" rev-parse --verify --quiet "$ref" 2>/dev/null
+ if [ "$head" == "ref: refs/heads/.invalid" ]; then
+ __git rev-parse --verify --quiet "$ref"
return $?
fi
fi
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 2/5] t9902: verify that completion does not print anything
From: Patrick Steinhardt @ 2024-01-15 10:36 UTC (permalink / raw)
To: git
Cc: Stan Hu, SZEDER Gábor, Jeff King, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <cover.1705314554.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]
The Bash completion script must not print anything to either stdout or
stderr. Instead, it is only expected to populate certain variables.
Tighten our `test_completion ()` test helper to verify this requirement.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t9902-completion.sh | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index aa9a614de3..95ec762a74 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -5,6 +5,12 @@
test_description='test bash completion'
+# The Bash completion scripts must not print anything to either stdout or
+# stderr, which we try to verify. When tracing is enabled without support for
+# BASH_XTRACEFD this assertion will fail, so we have to mark the test as
+# untraceable with such ancient Bash versions.
+test_untraceable=UnfortunatelyYes
+
. ./lib-bash.sh
complete ()
@@ -87,9 +93,11 @@ test_completion ()
else
sed -e 's/Z$//' |sort >expected
fi &&
- run_completion "$1" &&
+ run_completion "$1" >"$TRASH_DIRECTORY"/bash-completion-output 2>&1 &&
sort out >out_sorted &&
- test_cmp expected out_sorted
+ test_cmp expected out_sorted &&
+ test_must_be_empty "$TRASH_DIRECTORY"/bash-completion-output &&
+ rm "$TRASH_DIRECTORY"/bash-completion-output
}
# Test __gitcomp.
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 1/5] completion: discover repo path in `__git_pseudoref_exists ()`
From: Patrick Steinhardt @ 2024-01-15 10:35 UTC (permalink / raw)
To: git
Cc: Stan Hu, SZEDER Gábor, Jeff King, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <cover.1705314554.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]
The helper function `__git_pseudoref_exists ()` expects that the repo
path has already been discovered by its callers, which makes for a
rather fragile calling convention. Refactor the function to discover the
repo path itself to make it more self-contained, which also removes the
need to discover the path in some of its callers.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/completion/git-completion.bash | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8c40ade494..06a9107449 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -138,6 +138,8 @@ __git_pseudoref_exists ()
{
local ref=$1
+ __git_find_repo_path
+
# If the reftable is in use, we have to shell out to 'git rev-parse'
# to determine whether the ref exists instead of looking directly in
# the filesystem to determine whether the ref exists. Otherwise, use
@@ -1656,7 +1658,6 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options
_git_cherry_pick ()
{
- __git_find_repo_path
if __git_pseudoref_exists CHERRY_PICK_HEAD; then
__gitcomp "$__git_cherry_pick_inprogress_options"
return
@@ -2966,7 +2967,6 @@ _git_reset ()
_git_restore ()
{
- __git_find_repo_path
case "$prev" in
-s)
__git_complete_refs
@@ -2995,7 +2995,6 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options
_git_revert ()
{
- __git_find_repo_path
if __git_pseudoref_exists REVERT_HEAD; then
__gitcomp "$__git_revert_inprogress_options"
return
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 0/5] completion: silence pseudo-ref existence check
From: Patrick Steinhardt @ 2024-01-15 10:35 UTC (permalink / raw)
To: git
Cc: Stan Hu, SZEDER Gábor, Jeff King, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <cover.1704969119.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]
Hi,
this is the second version of my patch series that refactors the Bash
completion scripts to work better with reftables. Changes compared to
v1:
- I've incorporated SZEDER's feedback that he has sent in reply to the
original patch series that is already in `master`. Most importantly,
we now discover the repo path in `__git_pseudoref_exists ()` itself.
- I've included some refactorings to stop leaking local variables in
`__git_pseudoref_exists ()`.
- I've added tests for `__git_pseudoref_exists ()` directly, which was
also suggested by SZEDER.
- I've marked t9902 with `test_untraceable` now to unbreak macOS CI.
- I've refactored the completion test to use `git show-ref --exists`
to fix an issue with unborn pseudorefs that I didn't notice
previously.
I didn't include a range-diff because quite a lof of things have moved
around.
Patrick
Patrick Steinhardt (5):
completion: discover repo path in `__git_pseudoref_exists ()`
t9902: verify that completion does not print anything
completion: improve existence check for pseudo-refs
completion: silence pseudoref existence check
completion: treat dangling symrefs as existing pseudorefs
contrib/completion/git-completion.bash | 11 +++---
t/t9902-completion.sh | 47 ++++++++++++++++++++++++--
2 files changed, 50 insertions(+), 8 deletions(-)
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] clone: support cloning of filtered bundles
From: phillip.wood123 @ 2024-01-15 10:35 UTC (permalink / raw)
To: Nikolay Edigaryev, phillip.wood
Cc: Nikolay Edigaryev via GitGitGadget, git, Derrick Stolee
In-Reply-To: <CAFX5hXQ291gR4831dtRTdvtffWefCNCFp13ADvOkU7s3SVPczQ@mail.gmail.com>
Hi Nikolay
On 14/01/2024 21:26, Nikolay Edigaryev wrote:
>> Note that currently, when you clone a normal non-filtered bundle with a
>> '--filter' argument specified, no filtering will take place and no error
>> will be thrown. "promisor = true" and "partialclonefilter = ..." options
>> will be set in the repo config, but no .promisor file will be created.
>> This is even more confusing IMO, but that's how it currently on
>> Git 2.43.0.
>
> I was wrong about this one: the .promisor pack file actually gets created.
>
> And the filtering is not being done because the "uploadpack.allowFilter"
> global option is not enabled by default.
That makes sense - if you try to make a partial clone from a remote that
does not support object filtering we fallback to a full clone and print
the warning you mention below.
> Unfortunately the only way to figure this out is to run Git with
> GIT_TRACE=2, which shows:
That seems strange, you should see the warning without having to set
GIT_TRACE. I've certainly seen the warning in the past when trying to
create a partial clone from a remote did not support them without me
setting any special environment variables.
Best Wishes
Phillip
> warning: filtering not recognized by server, ignoring
>
> So please feel free to skip this part from the consideration.
>
>
> On Sun, Jan 14, 2024 at 11:39 PM Nikolay Edigaryev <edigaryev@gmail.com> wrote:
>>
>> Hello Phillip,
>>
>>> As I understand it if you're cloning from a bundle file then then there
>>> is no remote so how can we set a remote-specific config?
>>
>> There is a remote, for more details see df61c88979 (clone: also
>> configure url for bare clones, 2010-03-29), which has the following
>> code:
>>
>> strbuf_addf(&key, "remote.%s.url", remote_name);
>> git_config_set(key.buf, repo);
>> strbuf_reset(&key);
>>
>> You can verify this by creating a bundle on Git 2.43.0 with "git create
>> bundle bundle.bundle --all" and then cloning it with "git clone
>> --bare /path/to/bundle.bundle", in my case the following repo-wide
>> configuration file was created:
>>
>> [core]
>> repositoryformatversion = 0
>> filemode = true
>> bare = true
>> ignorecase = true
>> precomposeunicode = true
>> [remote "origin"]
>> url = /Users/edi/src/cirrus-cli/cli.bundle
>>
>>> I'm surprised that the proposed change does not require the user to pass
>>> "--filter" to "git clone" as I expected that we'd want to check that the
>>> filter on the command line was compatible with the filter used to create
>>> the bundle. Allowing "git clone" to create a partial clone without the
>>> user asking for it by passing the "--filter" option feels like is going
>>> to end up confusing users.
>>
>> Note that currently, when you clone a normal non-filtered bundle with a
>> '--filter' argument specified, no filtering will take place and no error
>> will be thrown. "promisor = true" and "partialclonefilter = ..." options
>> will be set in the repo config, but no .promisor file will be created.
>> This is even more confusing IMO, but that's how it currently on
>> Git 2.43.0.
>>
>> You have a good point, but I feel like completely preventing cloning of
>> filtered bundles and requiring a '--filter' argument is very taxing. If
>> you've already specified a '--filter' when creating a bundle (and thus
>> your intent to use partially cloned data), why do it multiple times?
>>
>> What I propose as an alternative here is to act based on the user's
>> intent when cloning:
>>
>> * when the user specifies no '--filter' argument, do nothing special,
>> allow cloning both types of bundles: normal and filtered (with the
>> logic from this patch)
>>
>> * when the user does specify a '--filter' argument, either:
>> * throw an error explaining that filtering of filtered bundles is not
>> supported
>> * or compare the user's filter specification and the one that is
>> in the bundle and only throw an error if they mismatch
>>
>> Let me know what you think about this (and perhaps you have a more
>> concrete example in mind where this will have negative consequences)
>> and I'll be happy to do a next iteration.
>>
>>
>> On Sun, Jan 14, 2024 at 10:00 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>
>>> Hi Nikolay
>>>
>>> On 14/01/2024 11:16, Nikolay Edigaryev via GitGitGadget wrote:
>>>> From: Nikolay Edigaryev <edigaryev@gmail.com>
>>>>
>>>> f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an
>>>> incredibly useful ability to create filtered bundles, which advances
>>>> the partial clone/promisor support in Git and allows for archiving
>>>> large repositories to object storages like S3 in bundles that are:
>>>>
>>>> * easy to manage
>>>> * bundle is just a single file, it's easier to guarantee atomic
>>>> replacements in object storages like S3 and they are faster to
>>>> fetch than a bare repository since there's only a single GET
>>>> request involved
>>>> * incredibly tiny
>>>> * no indexes (which may be more than 10 MB for some repositories)
>>>> and other fluff, compared to cloning a bare repository
>>>> * bundle can be filtered to only contain the tips of refs neccessary
>>>> for e.g. code-analysis purposes
>>>>
>>>> However, in 86fdd94d72 (clone: fail gracefully when cloning filtered
>>>> bundle, 2022-03-09) the cloning of such bundles was disabled, with a
>>>> note that this behavior is not desired, and it the long-term this
>>>> should be possible.
>>>>
>>>> The commit above states that it's not possible to have this at the
>>>> moment due to lack of remote and a repository-global config that
>>>> specifies an object filter, yet it's unclear why a remote-specific
>>>> config can't be used instead, which is what this change does.
>>>
>>> As I understand it if you're cloning from a bundle file then then there
>>> is no remote so how can we set a remote-specific config?
>>>
>>> I'm surprised that the proposed change does not require the user to pass
>>> "--filter" to "git clone" as I expected that we'd want to check that the
>>> filter on the command line was compatible with the filter used to create
>>> the bundle. Allowing "git clone" to create a partial clone without the
>>> user asking for it by passing the "--filter" option feels like is going
>>> to end up confusing users.
>>>
>>>> +test_expect_success 'cloning from filtered bundle works' '
>>>> + git bundle create partial.bdl --all --filter=blob:none &&
>>>> + git clone --bare partial.bdl partial 2>err
>>>
>>> The redirection hides any error message which will make debugging test
>>> failures harder. It would be nice to see this test check any config set
>>> when cloning and that git commands can run successfully in the repository.
>>>
>>> Best Wishes
>>>
>>> Phillip
^ permalink raw reply
* Re: [PATCH] clone: support cloning of filtered bundles
From: phillip.wood123 @ 2024-01-15 10:18 UTC (permalink / raw)
To: Nikolay Edigaryev, phillip.wood
Cc: Nikolay Edigaryev via GitGitGadget, git, Derrick Stolee
In-Reply-To: <CAFX5hXTCPt-rDrWZ-RN8S84o_FooY3Ck2H1kMYdHQGzuetPBSw@mail.gmail.com>
Hi Nikolay
On 14/01/2024 19:39, Nikolay Edigaryev wrote:
> Hello Phillip,
>
>> As I understand it if you're cloning from a bundle file then then there
>> is no remote so how can we set a remote-specific config?
>
> There is a remote, for more details see df61c88979 (clone: also
> configure url for bare clones, 2010-03-29), which has the following
> code:
>
> strbuf_addf(&key, "remote.%s.url", remote_name);
> git_config_set(key.buf, repo);
> strbuf_reset(&key);
>
> You can verify this by creating a bundle on Git 2.43.0 with "git create
> bundle bundle.bundle --all" and then cloning it with "git clone
> --bare /path/to/bundle.bundle", in my case the following repo-wide
> configuration file was created:
>
> [core]
> repositoryformatversion = 0
> filemode = true
> bare = true
> ignorecase = true
> precomposeunicode = true
> [remote "origin"]
> url = /Users/edi/src/cirrus-cli/cli.bundle
Oh, thanks for clarifying that I didn't realize we set "origin" to point
to the bundle. That means this patch creates a promisor remote config
pointing to a bundle that does not contain the missing objects. As Junio
said that doesn't make much sense to me as the point of the promisor
config is to allow git to lazily fetch the missing objects.
Best Wishes
Phillip
>> I'm surprised that the proposed change does not require the user to pass
>> "--filter" to "git clone" as I expected that we'd want to check that the
>> filter on the command line was compatible with the filter used to create
>> the bundle. Allowing "git clone" to create a partial clone without the
>> user asking for it by passing the "--filter" option feels like is going
>> to end up confusing users.
>
> Note that currently, when you clone a normal non-filtered bundle with a
> '--filter' argument specified, no filtering will take place and no error
> will be thrown. "promisor = true" and "partialclonefilter = ..." options
> will be set in the repo config, but no .promisor file will be created.
> This is even more confusing IMO, but that's how it currently on
> Git 2.43.0.
>
> You have a good point, but I feel like completely preventing cloning of
> filtered bundles and requiring a '--filter' argument is very taxing. If
> you've already specified a '--filter' when creating a bundle (and thus
> your intent to use partially cloned data), why do it multiple times?
>
> What I propose as an alternative here is to act based on the user's
> intent when cloning:
>
> * when the user specifies no '--filter' argument, do nothing special,
> allow cloning both types of bundles: normal and filtered (with the
> logic from this patch)
>
> * when the user does specify a '--filter' argument, either:
> * throw an error explaining that filtering of filtered bundles is not
> supported
> * or compare the user's filter specification and the one that is
> in the bundle and only throw an error if they mismatch
>
> Let me know what you think about this (and perhaps you have a more
> concrete example in mind where this will have negative consequences)
> and I'll be happy to do a next iteration.
>
>
> On Sun, Jan 14, 2024 at 10:00 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Nikolay
>>
>> On 14/01/2024 11:16, Nikolay Edigaryev via GitGitGadget wrote:
>>> From: Nikolay Edigaryev <edigaryev@gmail.com>
>>>
>>> f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an
>>> incredibly useful ability to create filtered bundles, which advances
>>> the partial clone/promisor support in Git and allows for archiving
>>> large repositories to object storages like S3 in bundles that are:
>>>
>>> * easy to manage
>>> * bundle is just a single file, it's easier to guarantee atomic
>>> replacements in object storages like S3 and they are faster to
>>> fetch than a bare repository since there's only a single GET
>>> request involved
>>> * incredibly tiny
>>> * no indexes (which may be more than 10 MB for some repositories)
>>> and other fluff, compared to cloning a bare repository
>>> * bundle can be filtered to only contain the tips of refs neccessary
>>> for e.g. code-analysis purposes
>>>
>>> However, in 86fdd94d72 (clone: fail gracefully when cloning filtered
>>> bundle, 2022-03-09) the cloning of such bundles was disabled, with a
>>> note that this behavior is not desired, and it the long-term this
>>> should be possible.
>>>
>>> The commit above states that it's not possible to have this at the
>>> moment due to lack of remote and a repository-global config that
>>> specifies an object filter, yet it's unclear why a remote-specific
>>> config can't be used instead, which is what this change does.
>>
>> As I understand it if you're cloning from a bundle file then then there
>> is no remote so how can we set a remote-specific config?
>>
>> I'm surprised that the proposed change does not require the user to pass
>> "--filter" to "git clone" as I expected that we'd want to check that the
>> filter on the command line was compatible with the filter used to create
>> the bundle. Allowing "git clone" to create a partial clone without the
>> user asking for it by passing the "--filter" option feels like is going
>> to end up confusing users.
>>
>>> +test_expect_success 'cloning from filtered bundle works' '
>>> + git bundle create partial.bdl --all --filter=blob:none &&
>>> + git clone --bare partial.bdl partial 2>err
>>
>> The redirection hides any error message which will make debugging test
>> failures harder. It would be nice to see this test check any config set
>> when cloning and that git commands can run successfully in the repository.
>>
>> Best Wishes
>>
>> Phillip
^ permalink raw reply
* Re: [PATCH v2 2/5] reftable/stack: refactor reloading to use file descriptor
From: Patrick Steinhardt @ 2024-01-15 10:03 UTC (permalink / raw)
To: Jeff King; +Cc: git, Han-Wen Nienhuys, Junio C Hamano
In-Reply-To: <20240114101424.GA1196682@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 3799 bytes --]
On Sun, Jan 14, 2024 at 05:14:24AM -0500, Jeff King wrote:
> On Thu, Jan 11, 2024 at 11:06:43AM +0100, Patrick Steinhardt wrote:
>
> > We're about to introduce a stat(3P)-based caching mechanism to reload
> > the list of stacks only when it has changed. In order to avoid race
> > conditions this requires us to have a file descriptor available that we
> > can use to call fstat(3P) on.
> >
> > Prepare for this by converting the code to use `fd_read_lines()` so that
> > we have the file descriptor readily available.
>
> Coverity noted a case with this series where we might feed a negative
> value to fstat(). I'm not sure if it's a bug or not.
>
> The issue is that here:
>
> > @@ -329,9 +330,19 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
> > if (tries > 3 && tv_cmp(&now, &deadline) >= 0)
> > goto out;
> >
> > - err = read_lines(st->list_file, &names);
> > - if (err < 0)
> > - goto out;
> > + fd = open(st->list_file, O_RDONLY);
> > + if (fd < 0) {
> > + if (errno != ENOENT) {
> > + err = REFTABLE_IO_ERROR;
> > + goto out;
> > + }
> > +
> > + names = reftable_calloc(sizeof(char *));
> > + } else {
> > + err = fd_read_lines(fd, &names);
> > + if (err < 0)
> > + goto out;
> > + }
>
> ...we might end up with fd as "-1" after calling open() on the list
> file. For most errors we'll jump to "out", which makes sense. But if we
> get ENOENT, we keep going with an empty file-list, which makes sense.
>
> But we then do other stuff with "fd". I think this case is OK:
>
> > @@ -356,12 +367,16 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
> > names = NULL;
> > free_names(names_after);
> > names_after = NULL;
> > + close(fd);
> > + fd = -1;
>
> We only get here if reftable_stack_reload_once() returned an error,
> which it won't do since we feed it a blank set of names (and anyway,
> close(-1) is a harmless noop).
>
> But if we actually get to the end of the function, it's more
> questionable. As of this patch, it's OK:
>
> > delay = delay + (delay * rand()) / RAND_MAX + 1;
> > sleep_millisec(delay);
> > }
> >
> > out:
> > + if (fd >= 0)
> > + close(fd);
> > free_names(names);
> > free_names(names_after);
> > return err;
>
> But in the next patch we have this hunk:
>
> > @@ -374,7 +375,11 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
> > sleep_millisec(delay);
> > }
> >
> > + stat_validity_update(&st->list_validity, fd);
> > +
> > out:
> > + if (err)
> > + stat_validity_clear(&st->list_validity);
> > if (fd >= 0)
> > close(fd);
> > free_names(names);
>
> which means we'll feed a negative value to stat_validity_update(). I
> think this may be OK, because I'd imagine the only sensible thing to do
> is call stat_validity_clear() instead. And using a negative fd means
> fstat() will fail, which will cause stat_validity_update() to clear the
> validity struct anyway. But I thought it was worth double-checking.
Good catch, and thanks a lot for double-checking. I was briefly
wondering whether this behaviour is actually specified by POSIX. In any
case, fstat(3P) explicitly documents `EBADF` as:
The fildes argument is not a valid file descriptor.
That makes me think that this code is indeed POSIX-compliant, as
implementations are expected to handle invalid file descriptors via this
error code.
So overall this works as intended, even though I would not consider it
to be the cleanest way to handle this. Unless you or others think that
this should be refactored I'll leave it as-is for now though.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Patrick Steinhardt @ 2024-01-15 9:02 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git, Stan Hu
In-Reply-To: <20240112151655.GA640039@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]
On Fri, Jan 12, 2024 at 10:16:55AM -0500, Jeff King wrote:
> On Fri, Jan 12, 2024 at 02:12:43PM +0100, Johannes Schindelin wrote:
>
> > But my main concern is: Why does this happen in the first place? If we are
> > running with Bash, why does `BASH_XTRACEFD` to work as intended here and
> > makes it necessary to filter out the traced commands?
>
> BASH_XTRACEFD was introduced in bash 4.1. macOS ships with the ancient
> bash 3.2.57, which is the last GPLv2 version.
>
> One simple solution is to mark the script with test_untraceable. See
> 5fc98e79fc (t: add means to disable '-x' tracing for individual test
> scripts, 2018-02-24) and 5827506928 (t1510-repo-setup: mark as
> untraceable with '-x', 2018-02-24).
>
> That will disable tracing entirely in the script for older versions of
> bash, which could make debugging harder. But it will still work as
> expected for people on reasonable versions of bash, and doesn't
> introduce any complicated code.
>
> -Peff
Ah, this is indeed the best solution. Thanks for the hints and
investigations everyone, will fix in the next iteration.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] push: improve consistency of output when "up to date"
From: Patrick Steinhardt @ 2024-01-15 8:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Sunshine, Benji Kay via GitGitGadget, git, Benji Kay
In-Reply-To: <xmqqjzofec0e.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]
On Thu, Jan 11, 2024 at 02:33:21PM -0800, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > Thanks. This particular change is proposed periodically...
> >
> >> diff --git a/transport.c b/transport.c
> >> @@ -1467,7 +1467,7 @@ int transport_push(struct repository *r,
> >> else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
> >> - fprintf(stderr, "Everything up-to-date\n");
> >> + fprintf(stderr, "Everything up to date.\n");
> >
> > ... but has not been considered desirable.
> >
> > See, for instance, this email thread explaining the rationale for
> > avoiding such a change:
> > https://lore.kernel.org/git/pull.1298.git.1658908927714.gitgitgadget@gmail.com/T/
>
> Looking at the "grep" hits:
>
> $ git grep -e 'up-to-date.*"' \*.c
> builtin/rm.c: OPT__FORCE(&force, N_("override the up-to-date check"), PARSE_OPT_NOCOMPLETE),
> builtin/send-pack.c: fprintf(stderr, "Everything up-to-date\n");
> http-push.c: fprintf(stderr, "'%s': up-to-date\n", ref->name);
> http-push.c: "Maybe you are not up-to-date and "
> transport.c: fprintf(stderr, "Everything up-to-date\n");
>
> it is true that these are not marked for translation, which should
> be a clue enough that we want them to be exactly the way they are
> spelled. However, they are going to the standard error stream. Is
> it reasonable to expect third-party tools scraping it to find the
> string "up-to-date"?
I would say it's not entirely reasonable:
- These are strings that users see frequently, and if they are not
proficient in the English language I think it actually regresses
their user experience.
- The way this string is written would never lead me, as a script
developer, to think that this is a message that should be parsed by
my script. It's simply too user-focussed to make me think so.
- Last but not least, I think it's not entirely unreasonable to ask
script developers to use e.g. LANG=C when they expect strings to be
stable.
Also, with the introduction of `git push --porcelain`, I think there is
even less reason to keep such user-visible strings intact. Any machine
that wants to parse output of git-push(1) should use `--porcelain`
instead in my opinion.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] commit-graph: fix memory leak when not writing graph
From: Patrick Steinhardt @ 2024-01-15 7:08 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
In-Reply-To: <ZZhUWu5pgBEYK409@nand.local>
[-- Attachment #1: Type: text/plain, Size: 957 bytes --]
On Fri, Jan 05, 2024 at 02:11:22PM -0500, Taylor Blau wrote:
> On Mon, Dec 18, 2023 at 11:02:28AM +0100, Patrick Steinhardt wrote:
> > When `write_commit_graph()` bails out writing a split commit-graph early
> > then it may happen that we have already gathered the set of existing
> > commit-graph file names without yet determining the new merged set of
> > files. This can result in a memory leak though because we only clear the
> > preimage of files when we have collected the postimage.
> >
> > Fix this issue by dropping the condition altogether so that we always
> > try to free both preimage and postimage filenames. As the context
> > structure is zero-initialized this simplification is safe to do.
>
> Looks obviously good to me, thanks for finding and fixing.
Cc'ing Junio so that this fix doesn't fall off the radar. I thought I
saw the topic in "seen" once, but either I misremember or it got dropped
from there.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH] bisect: add --force flag to force checkout
From: Kevin Wang via GitGitGadget @ 2024-01-15 7:05 UTC (permalink / raw)
To: git; +Cc: Kevin Wang, Kevin Wang
From: Kevin Wang <kevmo314@gmail.com>
Adds a `--force`/`-f` flag to `git bisect good/bad` and `git bisect run` to
force a checkout. Currently, if the repository state adds any local changes
the user must manually reset the repository state before moving to the next
bisection step. This can happen with package lock files or log output data,
for example. With this change, a developer can run `git bisect run --force`
to automatically reset the repository state after each evaluation. The flag
is also supported as `git bisect (good|bad) --force` as well.
Signed-off-by: Kevin Wang <kevmo314@gmail.com>
---
bisect: add --force flag to force checkout
Adds a --force/-f flag to git bisect good/bad and git bisect run to
force a checkout. Currently, if the repository state adds any local
changes the user must manually reset the repository state before moving
to the next bisection step. This can happen with package lock files or
log output data, for example. With this change, a developer can run git
bisect run --force to automatically reset the repository state after
each evaluation. The flag is also supported as git bisect (good|bad)
--force as well.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1641%2Fkevmo314%2Fbisect-force-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1641/kevmo314/bisect-force-v1
Pull-Request: https://github.com/git/git/pull/1641
Documentation/git-bisect.txt | 13 ++++-
bisect.c | 17 ++++--
bisect.h | 4 +-
builtin/bisect.c | 50 +++++++++++-----
t/t6030-bisect-porcelain.sh | 109 +++++++++++++++++++++++++++++++++++
5 files changed, 167 insertions(+), 26 deletions(-)
diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index aa02e462244..57357221718 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -19,14 +19,14 @@ on the subcommand:
git bisect start [--term-(new|bad)=<term-new> --term-(old|good)=<term-old>]
[--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]
git bisect (bad|new|<term-new>) [<rev>]
- git bisect (good|old|<term-old>) [<rev>...]
+ git bisect (good|old|<term-old>) [-f] [<rev>...]
git bisect terms [--term-good | --term-bad]
git bisect skip [(<rev>|<range>)...]
git bisect reset [<commit>]
git bisect (visualize|view)
git bisect replay <logfile>
git bisect log
- git bisect run <cmd> [<arg>...]
+ git bisect run [-f] <cmd> [<arg>...]
git bisect help
This command uses a binary search algorithm to find which commit in
@@ -381,6 +381,15 @@ ignored.
This option is particularly useful in avoiding false positives when a merged
branch contained broken or non-buildable commits, but the merge itself was OK.
+-f::
+--force::
++
+Throw away any local changes and untracked files before moving to the next
+bisection step.
++
+This option may be useful if the repository state changes when testing a
+revision.
+
EXAMPLES
--------
diff --git a/bisect.c b/bisect.c
index 985b96ed132..72ce09f2015 100644
--- a/bisect.c
+++ b/bisect.c
@@ -713,7 +713,7 @@ static int is_expected_rev(const struct object_id *oid)
}
enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
- int no_checkout)
+ int no_checkout, int force_checkout)
{
struct commit *commit;
struct pretty_print_context pp = {0};
@@ -728,8 +728,13 @@ enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
struct child_process cmd = CHILD_PROCESS_INIT;
cmd.git_cmd = 1;
- strvec_pushl(&cmd.args, "checkout", "-q",
- oid_to_hex(bisect_rev), "--", NULL);
+ if (force_checkout) {
+ strvec_pushl(&cmd.args, "checkout", "-f", "-q",
+ oid_to_hex(bisect_rev), "--", NULL);
+ } else {
+ strvec_pushl(&cmd.args, "checkout", "-q",
+ oid_to_hex(bisect_rev), "--", NULL);
+ }
if (run_command(&cmd))
/*
* Errors in `run_command()` itself, signaled by res < 0,
@@ -850,7 +855,7 @@ static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int
handle_skipped_merge_base(mb);
} else {
printf(_("Bisecting: a merge base must be tested\n"));
- res = bisect_checkout(mb, no_checkout);
+ res = bisect_checkout(mb, no_checkout, 0);
if (!res)
/* indicate early success */
res = BISECT_INTERNAL_SUCCESS_MERGE_BASE;
@@ -1002,7 +1007,7 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
* the end of bisect_helper::cmd_bisect__helper() helps bypassing
* all the code related to finding a commit to test.
*/
-enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
+enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int force_checkout)
{
struct strvec rev_argv = STRVEC_INIT;
struct rev_info revs = REV_INFO_INIT;
@@ -1104,7 +1109,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
/* Clean up objects used, as they will be reused. */
repo_clear_commit_marks(r, ALL_REV_FLAGS);
- res = bisect_checkout(bisect_rev, no_checkout);
+ res = bisect_checkout(bisect_rev, no_checkout, force_checkout);
cleanup:
release_revisions(&revs);
strvec_clear(&rev_argv);
diff --git a/bisect.h b/bisect.h
index ee3fd65f3bd..6f972365faa 100644
--- a/bisect.h
+++ b/bisect.h
@@ -71,7 +71,7 @@ struct bisect_state {
unsigned int nr_bad;
};
-enum bisect_error bisect_next_all(struct repository *r, const char *prefix);
+enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int force_checkout);
int estimate_bisect_steps(int all);
@@ -80,6 +80,6 @@ void read_bisect_terms(const char **bad, const char **good);
int bisect_clean_state(void);
enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
- int no_checkout);
+ int no_checkout, int force_checkout);
#endif
diff --git a/builtin/bisect.c b/builtin/bisect.c
index 47fcce59ff7..99c16d57942 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -29,7 +29,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
" [--no-checkout] [--first-parent] [<bad> [<good>...]] [--]" \
" [<pathspec>...]")
#define BUILTIN_GIT_BISECT_STATE_USAGE \
- N_("git bisect (good|bad) [<rev>...]")
+ N_("git bisect (good|bad) [-f] [<rev>...]")
#define BUILTIN_GIT_BISECT_TERMS_USAGE \
"git bisect terms [--term-good | --term-bad]"
#define BUILTIN_GIT_BISECT_SKIP_USAGE \
@@ -45,7 +45,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
#define BUILTIN_GIT_BISECT_LOG_USAGE \
"git bisect log"
#define BUILTIN_GIT_BISECT_RUN_USAGE \
- N_("git bisect run <cmd> [<arg>...]")
+ N_("git bisect run [-f] <cmd> [<arg>...]")
static const char * const git_bisect_usage[] = {
BUILTIN_GIT_BISECT_START_USAGE,
@@ -651,7 +651,7 @@ static int bisect_successful(struct bisect_terms *terms)
return res;
}
-static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
+static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix, int force_checkout)
{
enum bisect_error res;
@@ -662,7 +662,7 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
return BISECT_FAILED;
/* Perform all bisection computation */
- res = bisect_next_all(the_repository, prefix);
+ res = bisect_next_all(the_repository, prefix, force_checkout);
if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
res = bisect_successful(terms);
@@ -674,14 +674,14 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
return res;
}
-static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
+static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix, int force_checkout)
{
if (bisect_next_check(terms, NULL)) {
bisect_print_status(terms);
return BISECT_OK;
}
- return bisect_next(terms, prefix);
+ return bisect_next(terms, prefix, force_checkout);
}
static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
@@ -875,7 +875,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
if (res)
return res;
- res = bisect_auto_next(terms, NULL);
+ res = bisect_auto_next(terms, NULL, 0);
if (!is_bisect_success(res))
bisect_clean_state();
return res;
@@ -917,7 +917,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
const char **argv)
{
const char *state;
- int i, verify_expected = 1;
+ int i, force_checkout = 0, verify_expected = 1;
struct object_id oid, expected;
struct oid_array revs = OID_ARRAY_INIT;
@@ -934,6 +934,13 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
argv++;
argc--;
+
+ if (argc > 0 && (!strcmp(argv[0], "--force") || !strcmp(argv[0], "-f"))) {
+ force_checkout = 1;
+ argv++;
+ argc--;
+ }
+
if (argc > 1 && !strcmp(state, terms->term_bad))
return error(_("'git bisect %s' can take only one argument."), terms->term_bad);
@@ -989,7 +996,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
}
oid_array_clear(&revs);
- return bisect_auto_next(terms, NULL);
+ return bisect_auto_next(terms, NULL, force_checkout);
}
static enum bisect_error bisect_log(void)
@@ -1078,7 +1085,7 @@ static enum bisect_error bisect_replay(struct bisect_terms *terms, const char *f
if (res)
return BISECT_FAILED;
- return bisect_auto_next(terms, NULL);
+ return bisect_auto_next(terms, NULL, 0);
}
static enum bisect_error bisect_skip(struct bisect_terms *terms, int argc,
@@ -1173,7 +1180,7 @@ static int do_bisect_run(const char *command)
return run_command(&cmd);
}
-static int verify_good(const struct bisect_terms *terms, const char *command)
+static int verify_good(const struct bisect_terms *terms, const char *command, int force_checkout)
{
int rc;
enum bisect_error res;
@@ -1189,13 +1196,13 @@ static int verify_good(const struct bisect_terms *terms, const char *command)
if (read_ref(no_checkout ? "BISECT_HEAD" : "HEAD", ¤t_rev))
return -1;
- res = bisect_checkout(&good_rev, no_checkout);
+ res = bisect_checkout(&good_rev, no_checkout, force_checkout);
if (res != BISECT_OK)
return -1;
rc = do_bisect_run(command);
- res = bisect_checkout(¤t_rev, no_checkout);
+ res = bisect_checkout(¤t_rev, no_checkout, force_checkout);
if (res != BISECT_OK)
return -1;
@@ -1209,6 +1216,7 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
const char *new_state;
int temporary_stdout_fd, saved_stdout;
int is_first_run = 1;
+ int force_checkout = 0;
if (bisect_next_check(terms, NULL))
return BISECT_FAILED;
@@ -1218,8 +1226,14 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
return BISECT_FAILED;
}
+ if (argc > 0 && (!strcmp(argv[0], "--force") || !strcmp(argv[0], "-f"))) {
+ force_checkout = 1;
+ argv++;
+ argc--;
+ }
sq_quote_argv(&command, argv);
strbuf_ltrim(&command);
+
while (1) {
res = do_bisect_run(command.buf);
@@ -1231,7 +1245,7 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
* missing or non-executable script.
*/
if (is_first_run && (res == 126 || res == 127)) {
- int rc = verify_good(terms, command.buf);
+ int rc = verify_good(terms, command.buf, force_checkout);
is_first_run = 0;
if (rc < 0 || 128 <= rc) {
error(_("unable to verify %s on good"
@@ -1271,7 +1285,11 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
saved_stdout = dup(1);
dup2(temporary_stdout_fd, 1);
- res = bisect_state(terms, 1, &new_state);
+ if (force_checkout) {
+ res = bisect_state(terms, 2, (const char *[]){ new_state, "--force" });
+ } else {
+ res = bisect_state(terms, 1, &new_state);
+ }
fflush(stdout);
dup2(saved_stdout, 1);
@@ -1342,7 +1360,7 @@ static int cmd_bisect__next(int argc, const char **argv UNUSED, const char *pref
return error(_("'%s' requires 0 arguments"),
"git bisect next");
get_terms(&terms);
- res = bisect_next(&terms, prefix);
+ res = bisect_next(&terms, prefix, 0);
free_terms(&terms);
return res;
}
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 561080bf240..eb17cc58a16 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1259,4 +1259,113 @@ test_expect_success 'verify correct error message' '
grep "git bisect good.*exited with error code" error
'
+test_expect_success 'bisect dirty, explicit ref' '
+ test_when_finished "git bisect reset" &&
+ add_line_into_file "Commit 1" file &&
+ add_line_into_file "Commit 2" file &&
+ add_line_into_file "Commit 3" file &&
+ git bisect start &&
+ git bisect bad HEAD &&
+ echo "modified state" >file &&
+ test_when_finished "git checkout -- file" &&
+ test_must_fail git bisect good HEAD~2
+'
+
+test_expect_success 'bisect dirty good force, explicit ref' '
+ test_when_finished "git bisect reset" &&
+ add_line_into_file "Commit 1" file &&
+ add_line_into_file "Commit 2" file &&
+ add_line_into_file "Commit 3" file &&
+ git bisect start &&
+ git bisect bad HEAD &&
+ echo "modified state" >file &&
+ test_when_finished "git checkout -- file" &&
+ git bisect good --force HEAD~2
+'
+
+test_expect_success 'bisect dirty, implicit ref' '
+ test_when_finished "git bisect reset" &&
+ add_line_into_file "Commit 1" file &&
+ add_line_into_file "Commit 2" file &&
+ add_line_into_file "Commit 3" file &&
+ add_line_into_file "Commit 4" file &&
+ add_line_into_file "Commit 5" file &&
+ git bisect start &&
+ git bisect bad HEAD &&
+ git bisect good HEAD~4 &&
+ echo "modified state" >file &&
+ test_when_finished "git checkout -- file" &&
+ test_must_fail git bisect good &&
+ test_must_fail git bisect bad
+'
+
+test_expect_success 'bisect dirty good force, implicit ref' '
+ test_when_finished "git bisect reset" &&
+ add_line_into_file "Commit 1" file &&
+ add_line_into_file "Commit 2" file &&
+ add_line_into_file "Commit 3" file &&
+ add_line_into_file "Commit 4" file &&
+ add_line_into_file "Commit 5" file &&
+ git bisect start &&
+ git bisect bad HEAD &&
+ git bisect good HEAD~4 &&
+ echo "modified state" >file &&
+ test_when_finished "git checkout -- file" &&
+ git bisect good --force
+'
+
+test_expect_success 'bisect dirty bad force, implicit ref' '
+ test_when_finished "git bisect reset" &&
+ add_line_into_file "Commit 1" file &&
+ add_line_into_file "Commit 2" file &&
+ add_line_into_file "Commit 3" file &&
+ add_line_into_file "Commit 4" file &&
+ add_line_into_file "Commit 5" file &&
+ git bisect start &&
+ git bisect bad HEAD &&
+ git bisect good HEAD~4 &&
+ echo "modified state" >file &&
+ test_when_finished "git checkout -- file" &&
+ git bisect bad --force
+'
+
+test_expect_success 'bisect run dirty' '
+ test_when_finished "git bisect reset" &&
+ test_when_finished "git checkout -- file" &&
+ add_line_into_file "Bisect run 1" file &&
+ add_line_into_file "Bisect run 2" file &&
+ add_line_into_file "Bisect run 3" file &&
+ add_line_into_file "Bisect run 4" file &&
+ add_line_into_file "Bisect run 5" file &&
+ write_script test_script.sh <<-\EOF &&
+ ! echo "modified state" >file
+ ! grep "Bisect run 3" || exit 126 >/dev/null
+ EOF
+ git bisect start &&
+ git bisect bad HEAD &&
+ git bisect good HEAD~4 &&
+ test_must_fail git bisect run ./test_script.sh
+'
+
+test_expect_success 'bisect run dirty force' '
+ test_when_finished "git bisect reset" &&
+ test_when_finished "git checkout -- file" &&
+ add_line_into_file "Bisect run 1" file &&
+ add_line_into_file "Bisect run 2" file &&
+ add_line_into_file "Bisect run 3" file &&
+ add_line_into_file "Bisect run 4" file &&
+ add_line_into_file "Bisect run 5" file &&
+ write_script test_script.sh <<-\EOF &&
+ ! echo "modified state" >file
+ ! grep "Bisect run 3" || exit 126 >/dev/null
+ EOF
+ HASH5=$(git rev-parse --verify HEAD) &&
+ git bisect start &&
+ git bisect bad HEAD &&
+ git bisect good HEAD~4 &&
+ echo "doing run\n" &&
+ git bisect run --force ./test_script.sh >my_bisect_log.txt &&
+ grep "$HASH5 is the first bad commit" my_bisect_log.txt
+'
+
test_done
base-commit: a26002b62827b89a19b1084bd75d9371d565d03c
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] clone: support cloning of filtered bundles
From: Junio C Hamano @ 2024-01-15 2:09 UTC (permalink / raw)
To: Nikolay Edigaryev via GitGitGadget; +Cc: git, Derrick Stolee, Nikolay Edigaryev
In-Reply-To: <xmqqcyu35rg9.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Nikolay Edigaryev via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index c6357af9498..4b3fedf78ed 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -1227,9 +1227,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>
>> if (fd > 0)
>> close(fd);
>> +
>> + if (has_filter) {
>> + strbuf_addf(&key, "remote.%s.promisor", remote_name);
>> + git_config_set(key.buf, "true");
>> + strbuf_reset(&key);
>> +
>> + strbuf_addf(&key, "remote.%s.partialclonefilter", remote_name);
>> + git_config_set(key.buf, expand_list_objects_filter_spec(&header.filter));
>> + strbuf_reset(&key);
>> + }
>> +
>
>> -# NEEDSWORK: 'git clone --bare' should be able to clone from a filtered
>> -# bundle, but that requires a change to promisor/filter config options.
> ...
> But a bundle that were created with objects _omitted_ already?
> ... the source of this clone operation, i.e. the bundle file that is
> pointed at by "remote.$remote_name.url", cannot be that promisor.
Extending the above a bit, one important way a bundle is used is as
a medium for sneaker-net. Instead of making a full clone over the
network, if you can create a bundle that records all objects and all
refs out of the source repository and then unbundle it in a
different place to create a repository, you can tweak the resulting
repository by either adding a separete remote or changing the
remote.origin.url so that your subsequent fetch goes over the
network to the repository you took the initial bundle from.
The "tweak the resulting repository" part however MUST be done
manually with the current system. If we can optionally record the
publically reachable URL of the source repository when we create a
bundle file, and "git clone" on the receiving side can read the URL
out of the bundle and act on it (e.g., show it to the user and offer
to record it as remote.origin.url in the resulting repository---I do
not think it is wise to do this silently without letting the user
know from security's point of view), then the use of bundle files as
a medium for sneaker-netting will become even easier.
And once that is done, perhaps allowing a filtered bundle to act as
a sneaker-net medium to simulate an initial filtered clone would
make sense. The promisor as well as the origin will be the network
reachable URL and subsequent fetches (both deliberate ones via "git
fetch" as well as lazy on-demand ones that backfills missing objects
via the "promisor" access) would become possible.
But without such a change to the bundle file format, allowing
"clone" to finish and pretend the resulting repository is usable is
somewhat irresponsible to the users. The on-demand lazy fetch would
fail after this code cloned from such a filtered bundle, no?
^ permalink raw reply
* Re: [PATCH] clone: support cloning of filtered bundles
From: Junio C Hamano @ 2024-01-15 1:13 UTC (permalink / raw)
To: Nikolay Edigaryev via GitGitGadget; +Cc: git, Derrick Stolee, Nikolay Edigaryev
In-Reply-To: <pull.1644.git.git.1705231010118.gitgitgadget@gmail.com>
"Nikolay Edigaryev via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> diff --git a/builtin/clone.c b/builtin/clone.c
> index c6357af9498..4b3fedf78ed 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1227,9 +1227,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>
> if (fd > 0)
> close(fd);
> +
> + if (has_filter) {
> + strbuf_addf(&key, "remote.%s.promisor", remote_name);
> + git_config_set(key.buf, "true");
> + strbuf_reset(&key);
> +
> + strbuf_addf(&key, "remote.%s.partialclonefilter", remote_name);
> + git_config_set(key.buf, expand_list_objects_filter_spec(&header.filter));
> + strbuf_reset(&key);
> + }
> +
> -# NEEDSWORK: 'git clone --bare' should be able to clone from a filtered
> -# bundle, but that requires a change to promisor/filter config options.
Sorry, but this "should be able to" does not make sense to me in the
first place.
I can understand how an operation to create a narrow clone of an
unfiltered bundle and then prepare the resulting repository for
future "fattening" by naming the unfiltered bundle file a
"promisor", and allow the user to lazily fetch objects that have
initially been filtered out of the bundle lazily.
But a bundle that were created with objects _omitted_ already? It
obviously cannot "promise" to deliber any objects that ought to be
reachable from the objects contained in it, so setting the bundle
file as promisor in the configuration does not help the resulting
repository. Those missing objects must be obtained from somewhere
other than that original "filtered" bundle, and if that source of
objects that can promise all the objects that are ought to be
reachable were specified as the promisor, it would make sense. But
the source of this clone operation, i.e. the bundle file that is
pointed at by "remote.$remote_name.url", cannot be that promisor.
^ permalink raw reply
* [PATCH v2 4/4] maintenance: use XDG config if it exists
From: Kristoffer Haugsbakk @ 2024-01-14 21:43 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, ps, stolee, Eric Sunshine, Taylor Blau
In-Reply-To: <cover.1705267839.git.code@khaugsbakk.name>
`git maintenance register` registers the repository in the user's global
config. `$XDG_CONFIG_HOME/git/config` is supposed to be used if
`~/.gitconfig` does not exist. However, this command creates a
`~/.gitconfig` file and writes to that one even though the XDG variant
exists.
This used to work correctly until 50a044f1e4 (gc: replace config
subprocesses with API calls, 2022-09-27), when the command started calling
the config API instead of git-config(1).
Also change `unregister` accordingly.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2:
• Add `unregister` tests
• Use subshell when exporting an env. variable
• Style in tests
• Free variables properly
builtin/gc.c | 27 ++++++++++++-------------
t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+), 14 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index c078751824c..cb80ced6cb5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1543,19 +1543,18 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
if (!found) {
int rc;
- char *user_config = NULL, *xdg_config = NULL;
+ char *global_config_file = NULL;
if (!config_file) {
- git_global_config_paths(&user_config, &xdg_config);
- config_file = user_config;
- if (!user_config)
- die(_("$HOME not set"));
+ global_config_file = git_global_config();
+ config_file = global_config_file;
}
+ if (!config_file)
+ die(_("$HOME not set"));
rc = git_config_set_multivar_in_file_gently(
config_file, "maintenance.repo", maintpath,
CONFIG_REGEX_NONE, 0);
- free(user_config);
- free(xdg_config);
+ free(global_config_file);
if (rc)
die(_("unable to add '%s' value of '%s'"),
@@ -1612,18 +1611,18 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
if (found) {
int rc;
- char *user_config = NULL, *xdg_config = NULL;
+ char *global_config_file = NULL;
+
if (!config_file) {
- git_global_config_paths(&user_config, &xdg_config);
- config_file = user_config;
- if (!user_config)
- die(_("$HOME not set"));
+ global_config_file = git_global_config();
+ config_file = global_config_file;
}
+ if (!config_file)
+ die(_("$HOME not set"));
rc = git_config_set_multivar_in_file_gently(
config_file, key, NULL, maintpath,
CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
- free(user_config);
- free(xdg_config);
+ free(global_config_file);
if (rc &&
(!force || rc == CONFIG_NOTHING_SET))
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 00d29871e65..0943dfa18a3 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -67,6 +67,51 @@ test_expect_success 'maintenance.auto config option' '
test_subcommand ! git maintenance run --auto --quiet <false
'
+test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' '
+ test_when_finished rm -r .config/git/config &&
+ (
+ XDG_CONFIG_HOME=.config &&
+ export XDG_CONFIG_HOME &&
+ mkdir -p $XDG_CONFIG_HOME/git &&
+ >$XDG_CONFIG_HOME/git/config &&
+ git maintenance register &&
+ git config --file=$XDG_CONFIG_HOME/git/config --get maintenance.repo >actual &&
+ pwd >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'register does not need XDG_CONFIG_HOME config to exist' '
+ test_when_finished git maintenance unregister &&
+ test_path_is_missing $XDG_CONFIG_HOME/git/config &&
+ git maintenance register &&
+ git config --global --get maintenance.repo >actual &&
+ pwd >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'unregister uses XDG_CONFIG_HOME config if it exists' '
+ test_when_finished rm -r .config/git/config &&
+ (
+ XDG_CONFIG_HOME=.config &&
+ export XDG_CONFIG_HOME &&
+ mkdir -p $XDG_CONFIG_HOME/git &&
+ >$XDG_CONFIG_HOME/git/config &&
+ git maintenance register &&
+ git maintenance unregister &&
+ test_must_fail git config --file=$XDG_CONFIG_HOME/git/config --get maintenance.repo >actual &&
+ test_must_be_empty actual
+ )
+'
+
+test_expect_success 'unregister does not need XDG_CONFIG_HOME config to exist' '
+ test_path_is_missing $XDG_CONFIG_HOME/git/config &&
+ git maintenance register &&
+ git maintenance unregister &&
+ test_must_fail git config --global --get maintenance.repo >actual &&
+ test_must_be_empty actual
+'
+
test_expect_success 'maintenance.<task>.enabled' '
git config maintenance.gc.enabled false &&
git config maintenance.commit-graph.enabled true &&
--
2.43.0
^ permalink raw reply related
* [PATCH v2 3/4] config: factor out global config file retrieval
From: Kristoffer Haugsbakk @ 2024-01-14 21:43 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, ps, stolee, Eric Sunshine, Taylor Blau
In-Reply-To: <cover.1705267839.git.code@khaugsbakk.name>
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”.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2:
• Don’t die; return `NULL`
builtin/config.c | 25 +++----------------------
config.c | 20 ++++++++++++++++++++
config.h | 4 ++++
3 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 6fff2655816..08fe36d4997 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -708,30 +708,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
if (use_global_config) {
- char *user_config, *xdg_config;
-
- git_global_config_paths(&user_config, &xdg_config);
- if (!user_config)
- /*
- * It is unknown if HOME/.gitconfig exists, so
- * we do not know if we should write to XDG
- * location; error out even if XDG_CONFIG_HOME
- * is set and points at a sane location.
- */
+ given_config_source.file = git_global_config();
+ if (!given_config_source.file)
die(_("$HOME not set"));
-
given_config_source.scope = CONFIG_SCOPE_GLOBAL;
-
- if (access_or_warn(user_config, R_OK, 0) &&
- xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
- given_config_source.file = xdg_config;
- free(user_config);
- } else {
- given_config_source.file = user_config;
- free(xdg_config);
- }
- }
- else if (use_system_config) {
+ } else if (use_system_config) {
given_config_source.file = git_system_config();
given_config_source.scope = CONFIG_SCOPE_SYSTEM;
} else if (use_local_config) {
diff --git a/config.c b/config.c
index ebc6a57e1c3..3cfeb3d8bd9 100644
--- a/config.c
+++ b/config.c
@@ -1987,6 +1987,26 @@ char *git_system_config(void)
return system_config;
}
+char *git_global_config(void)
+{
+ char *user_config, *xdg_config;
+
+ git_global_config_paths(&user_config, &xdg_config);
+ if (!user_config) {
+ free(xdg_config);
+ return NULL;
+ }
+
+ if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
+ !access_or_warn(xdg_config, R_OK, 0)) {
+ free(user_config);
+ return xdg_config;
+ } else {
+ free(xdg_config);
+ return user_config;
+ }
+}
+
void git_global_config_paths(char **user_out, char **xdg_out)
{
char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
diff --git a/config.h b/config.h
index e5e523553cc..625e932b993 100644
--- a/config.h
+++ b/config.h
@@ -382,6 +382,10 @@ int config_error_nonbool(const char *);
#endif
char *git_system_config(void);
+/**
+ * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.
+ */
+char *git_global_config(void);
void git_global_config_paths(char **user, char **xdg);
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
--
2.43.0
^ permalink raw reply related
* [PATCH v2 2/4] config: rename global config function
From: Kristoffer Haugsbakk @ 2024-01-14 21:43 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, ps, stolee, Eric Sunshine, Taylor Blau
In-Reply-To: <cover.1705267839.git.code@khaugsbakk.name>
Rename this function to a more descriptive name since we want to use the
existing name for a new function.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
builtin/config.c | 2 +-
builtin/gc.c | 4 ++--
builtin/var.c | 2 +-
config.c | 4 ++--
config.h | 2 +-
5 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 87d0dc92d99..6fff2655816 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -710,7 +710,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
if (use_global_config) {
char *user_config, *xdg_config;
- git_global_config(&user_config, &xdg_config);
+ git_global_config_paths(&user_config, &xdg_config);
if (!user_config)
/*
* It is unknown if HOME/.gitconfig exists, so
diff --git a/builtin/gc.c b/builtin/gc.c
index 7c11d5ebef0..c078751824c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1546,7 +1546,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
char *user_config = NULL, *xdg_config = NULL;
if (!config_file) {
- git_global_config(&user_config, &xdg_config);
+ git_global_config_paths(&user_config, &xdg_config);
config_file = user_config;
if (!user_config)
die(_("$HOME not set"));
@@ -1614,7 +1614,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
int rc;
char *user_config = NULL, *xdg_config = NULL;
if (!config_file) {
- git_global_config(&user_config, &xdg_config);
+ git_global_config_paths(&user_config, &xdg_config);
config_file = user_config;
if (!user_config)
die(_("$HOME not set"));
diff --git a/builtin/var.c b/builtin/var.c
index 8cf7dd9e2e5..cf5567208a2 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -90,7 +90,7 @@ static char *git_config_val_global(int ident_flag UNUSED)
char *user, *xdg;
size_t unused;
- git_global_config(&user, &xdg);
+ git_global_config_paths(&user, &xdg);
if (xdg && *xdg) {
normalize_path_copy(xdg, xdg);
strbuf_addf(&buf, "%s\n", xdg);
diff --git a/config.c b/config.c
index d26e16e3ce3..ebc6a57e1c3 100644
--- a/config.c
+++ b/config.c
@@ -1987,7 +1987,7 @@ char *git_system_config(void)
return system_config;
}
-void git_global_config(char **user_out, char **xdg_out)
+void git_global_config_paths(char **user_out, char **xdg_out)
{
char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
char *xdg_config = NULL;
@@ -2040,7 +2040,7 @@ static int do_git_config_sequence(const struct config_options *opts,
data, CONFIG_SCOPE_SYSTEM,
NULL);
- git_global_config(&user_config, &xdg_config);
+ git_global_config_paths(&user_config, &xdg_config);
if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
ret += git_config_from_file_with_options(fn, xdg_config, data,
diff --git a/config.h b/config.h
index 14f881ecfaf..e5e523553cc 100644
--- a/config.h
+++ b/config.h
@@ -382,7 +382,7 @@ int config_error_nonbool(const char *);
#endif
char *git_system_config(void);
-void git_global_config(char **user, char **xdg);
+void git_global_config_paths(char **user, char **xdg);
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
--
2.43.0
^ permalink raw reply related
* [PATCH v2 1/4] config: format newlines
From: Kristoffer Haugsbakk @ 2024-01-14 21:43 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, ps, stolee, Eric Sunshine, Taylor Blau
In-Reply-To: <cover.1705267839.git.code@khaugsbakk.name>
Remove unneeded newlines according to `clang-format`.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
Honestly the formatter changing these lines over and over again was just
annoying. And we're visiting the file anyway.
builtin/config.c | 1 -
config.c | 2 --
2 files changed, 3 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 11a4d4ef141..87d0dc92d99 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -760,7 +760,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)
given_config_source.scope = CONFIG_SCOPE_COMMAND;
}
-
if (respect_includes_opt == -1)
config_options.respect_includes = !given_config_source.file;
else
diff --git a/config.c b/config.c
index 9ff6ae1cb90..d26e16e3ce3 100644
--- a/config.c
+++ b/config.c
@@ -95,7 +95,6 @@ static long config_file_ftell(struct config_source *conf)
return ftell(conf->u.file);
}
-
static int config_buf_fgetc(struct config_source *conf)
{
if (conf->u.buf.pos < conf->u.buf.len)
@@ -3418,7 +3417,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
write_err_out:
ret = write_error(get_lock_file_path(&lock));
goto out_free;
-
}
void git_config_set_multivar_in_file(const char *config_filename,
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/4] maintenance: use XDG config if it exists
From: Kristoffer Haugsbakk @ 2024-01-14 21:43 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, ps, stolee, Eric Sunshine, Taylor Blau
In-Reply-To: <cover.1697660181.git.code@khaugsbakk.name>
I use the conventional XDG config path for the global configuration. This
path is always used except for `git maintenance register` and
`unregister`.
§ Changes since v1
• Free `config_file`
• https://lore.kernel.org/git/ZTZDsIcrB0zwHlFR@tanuki/
• Return `NULL` instead of dying
• https://lore.kernel.org/git/ZTZDqToqcsDiS5AP@tanuki/
• Tests
• Test unregister
• Use subshells
• Style
§ Patches
• 1–3: Preparatory
• 4: The desired change
§ CC
• Patrick Steinhardt: `config` changes; v1 feedback
• Derrick Stolee: `maintenance` changes
• Eric Sunshine: v1 feedback
• Taylor Blau: v1 feedback
Kristoffer Haugsbakk (4):
config: format newlines
config: rename global config function
config: factor out global config file retrieval
maintenance: use XDG config if it exists
builtin/config.c | 26 +++---------------------
builtin/gc.c | 27 ++++++++++++-------------
builtin/var.c | 2 +-
config.c | 26 ++++++++++++++++++++----
config.h | 6 +++++-
t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++
6 files changed, 89 insertions(+), 43 deletions(-)
Range-diff against v1:
1: 39934cb7e50 = 1: d5f6c8d62ec config: format newlines
2: 48a5357f97c = 2: cbc5fde0094 config: rename global config function
3: 147c767443c ! 3: 32e5ec7d866 config: factor out global config file retrieval
@@ Commit message
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
+
+ ## Notes (series) ##
+ v2:
+ • Don’t die; return `NULL`
+
## builtin/config.c ##
@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
}
@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix
- * location; error out even if XDG_CONFIG_HOME
- * is set and points at a sane location.
- */
-- die(_("$HOME not set"));
--
+ given_config_source.file = git_global_config();
++ if (!given_config_source.file)
+ die(_("$HOME not set"));
+-
given_config_source.scope = CONFIG_SCOPE_GLOBAL;
-
- if (access_or_warn(user_config, R_OK, 0) &&
@@ config.c: char *git_system_config(void)
+ char *user_config, *xdg_config;
+
+ git_global_config_paths(&user_config, &xdg_config);
-+ if (!user_config)
-+ /*
-+ * It is unknown if HOME/.gitconfig exists, so
-+ * we do not know if we should write to XDG
-+ * location; error out even if XDG_CONFIG_HOME
-+ * is set and points at a sane location.
-+ */
-+ die(_("$HOME not set"));
++ if (!user_config) {
++ free(xdg_config);
++ return NULL;
++ }
+
+ if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
+ !access_or_warn(xdg_config, R_OK, 0)) {
@@ config.h: int config_error_nonbool(const char *);
#endif
char *git_system_config(void);
++/**
++ * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.
++ */
+char *git_global_config(void);
void git_global_config_paths(char **user, char **xdg);
4: 1e2376a4b99 < -: ----------- maintenance: use XDG config if it exists
-: ----------- > 4: 8bd67c5bf01 maintenance: use XDG config if it exists
--
2.43.0
^ permalink raw reply
* Re: [PATCH] clone: support cloning of filtered bundles
From: Nikolay Edigaryev @ 2024-01-14 21:26 UTC (permalink / raw)
To: phillip.wood; +Cc: Nikolay Edigaryev via GitGitGadget, git, Derrick Stolee
In-Reply-To: <CAFX5hXTCPt-rDrWZ-RN8S84o_FooY3Ck2H1kMYdHQGzuetPBSw@mail.gmail.com>
> Note that currently, when you clone a normal non-filtered bundle with a
> '--filter' argument specified, no filtering will take place and no error
> will be thrown. "promisor = true" and "partialclonefilter = ..." options
> will be set in the repo config, but no .promisor file will be created.
> This is even more confusing IMO, but that's how it currently on
> Git 2.43.0.
I was wrong about this one: the .promisor pack file actually gets created.
And the filtering is not being done because the "uploadpack.allowFilter"
global option is not enabled by default.
Unfortunately the only way to figure this out is to run Git with
GIT_TRACE=2, which shows:
warning: filtering not recognized by server, ignoring
So please feel free to skip this part from the consideration.
On Sun, Jan 14, 2024 at 11:39 PM Nikolay Edigaryev <edigaryev@gmail.com> wrote:
>
> Hello Phillip,
>
> > As I understand it if you're cloning from a bundle file then then there
> > is no remote so how can we set a remote-specific config?
>
> There is a remote, for more details see df61c88979 (clone: also
> configure url for bare clones, 2010-03-29), which has the following
> code:
>
> strbuf_addf(&key, "remote.%s.url", remote_name);
> git_config_set(key.buf, repo);
> strbuf_reset(&key);
>
> You can verify this by creating a bundle on Git 2.43.0 with "git create
> bundle bundle.bundle --all" and then cloning it with "git clone
> --bare /path/to/bundle.bundle", in my case the following repo-wide
> configuration file was created:
>
> [core]
> repositoryformatversion = 0
> filemode = true
> bare = true
> ignorecase = true
> precomposeunicode = true
> [remote "origin"]
> url = /Users/edi/src/cirrus-cli/cli.bundle
>
> > I'm surprised that the proposed change does not require the user to pass
> > "--filter" to "git clone" as I expected that we'd want to check that the
> > filter on the command line was compatible with the filter used to create
> > the bundle. Allowing "git clone" to create a partial clone without the
> > user asking for it by passing the "--filter" option feels like is going
> > to end up confusing users.
>
> Note that currently, when you clone a normal non-filtered bundle with a
> '--filter' argument specified, no filtering will take place and no error
> will be thrown. "promisor = true" and "partialclonefilter = ..." options
> will be set in the repo config, but no .promisor file will be created.
> This is even more confusing IMO, but that's how it currently on
> Git 2.43.0.
>
> You have a good point, but I feel like completely preventing cloning of
> filtered bundles and requiring a '--filter' argument is very taxing. If
> you've already specified a '--filter' when creating a bundle (and thus
> your intent to use partially cloned data), why do it multiple times?
>
> What I propose as an alternative here is to act based on the user's
> intent when cloning:
>
> * when the user specifies no '--filter' argument, do nothing special,
> allow cloning both types of bundles: normal and filtered (with the
> logic from this patch)
>
> * when the user does specify a '--filter' argument, either:
> * throw an error explaining that filtering of filtered bundles is not
> supported
> * or compare the user's filter specification and the one that is
> in the bundle and only throw an error if they mismatch
>
> Let me know what you think about this (and perhaps you have a more
> concrete example in mind where this will have negative consequences)
> and I'll be happy to do a next iteration.
>
>
> On Sun, Jan 14, 2024 at 10:00 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >
> > Hi Nikolay
> >
> > On 14/01/2024 11:16, Nikolay Edigaryev via GitGitGadget wrote:
> > > From: Nikolay Edigaryev <edigaryev@gmail.com>
> > >
> > > f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an
> > > incredibly useful ability to create filtered bundles, which advances
> > > the partial clone/promisor support in Git and allows for archiving
> > > large repositories to object storages like S3 in bundles that are:
> > >
> > > * easy to manage
> > > * bundle is just a single file, it's easier to guarantee atomic
> > > replacements in object storages like S3 and they are faster to
> > > fetch than a bare repository since there's only a single GET
> > > request involved
> > > * incredibly tiny
> > > * no indexes (which may be more than 10 MB for some repositories)
> > > and other fluff, compared to cloning a bare repository
> > > * bundle can be filtered to only contain the tips of refs neccessary
> > > for e.g. code-analysis purposes
> > >
> > > However, in 86fdd94d72 (clone: fail gracefully when cloning filtered
> > > bundle, 2022-03-09) the cloning of such bundles was disabled, with a
> > > note that this behavior is not desired, and it the long-term this
> > > should be possible.
> > >
> > > The commit above states that it's not possible to have this at the
> > > moment due to lack of remote and a repository-global config that
> > > specifies an object filter, yet it's unclear why a remote-specific
> > > config can't be used instead, which is what this change does.
> >
> > As I understand it if you're cloning from a bundle file then then there
> > is no remote so how can we set a remote-specific config?
> >
> > I'm surprised that the proposed change does not require the user to pass
> > "--filter" to "git clone" as I expected that we'd want to check that the
> > filter on the command line was compatible with the filter used to create
> > the bundle. Allowing "git clone" to create a partial clone without the
> > user asking for it by passing the "--filter" option feels like is going
> > to end up confusing users.
> >
> > > +test_expect_success 'cloning from filtered bundle works' '
> > > + git bundle create partial.bdl --all --filter=blob:none &&
> > > + git clone --bare partial.bdl partial 2>err
> >
> > The redirection hides any error message which will make debugging test
> > failures harder. It would be nice to see this test check any config set
> > when cloning and that git commands can run successfully in the repository.
> >
> > Best Wishes
> >
> > Phillip
^ permalink raw reply
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