* Re: [GSoC] Use unsigned integral type for collection of bits
From: eugenio gigante @ 2024-02-19 23:43 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAFJh0PRJkVBr-A=UtmEcAh4cPgC3w_vdTPg6kkjgHVQXHTYRmA@mail.gmail.com>
On Sun, Feb 18, 2024 at 20:09 AM eric sunshine
<sunshine@sunshineco.com> wrote:
> The code in question is not being used as a "bag of bits". Rather,
> it's a tristate binary with values "not-set", "true", and "false".
> Whereas a typical binary could be represented by a single bit, this
> one needs the extra bit to handle the "not-set" case. Moreover, it is
> idiomatic in the Git codebase for -1 to represent "not-set", so I
> think this code is fine as-is since its meaning is clear to those
> familiar with the codebase, thus does not need any changes made to it.
Thank you for the clarification and sorry for the misunderstanding.
> So, refresh_index() is correctly expecting an unsigned value for
> `flags` but refresh() in `builtin/add.c` has undesirably declared
> `flags` as signed.
So, an unsigned type is preferable since we are dealing
with 'bags of bits', and probably only bitwise operators operate
on them. Also the mixing is not ideal.
Yes, I'm interested in fixing the one in `builtin/add.c`.
Il giorno mar 20 feb 2024 alle ore 00:39 eugenio gigante
<giganteeugenio2@gmail.com> ha scritto:
>
> On Sun, Feb 18, 2024 at 20:09 AM eric sunshine
> <sunshine@sunshineco.com> wrote:
> > The code in question is not being used as a "bag of bits". Rather,
> > it's a tristate binary with values "not-set", "true", and "false".
> > Whereas a typical binary could be represented by a single bit, this
> > one needs the extra bit to handle the "not-set" case. Moreover, it is
> > idiomatic in the Git codebase for -1 to represent "not-set", so I
> > think this code is fine as-is since its meaning is clear to those
> > familiar with the codebase, thus does not need any changes made to it.
>
> Thank you for the clarification and sorry for the misunderstanding.
>
> > So, refresh_index() is correctly expecting an unsigned value for
> > `flags` but refresh() in `builtin/add.c` has undesirably declared
> > `flags` as signed.
>
> So, an unsigned type is preferable since we are dealing
> with 'bags of bits', and probably only bitwise operators operate
> on them. Also the mixing is not ideal.
> Yes, I'm interested in fixing the one in `builtin/add.c`.
^ permalink raw reply
* Re: [PATCH 3/6] refs/files: sort reflogs returned by the reflog iterator
From: Junio C Hamano @ 2024-02-20 0:04 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <e4e4fac05c7f4bcac8ef96bdebb8a68eef40ead4.1708353264.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> We use a directory iterator to return reflogs via the reflog iterator.
> This iterator returns entries in the same order as readdir(3P) would and
> will thus yield reflogs with no discernible order.
>
> Set the new `DIR_ITERATOR_SORTED` flag that was introduced in the
> preceding commit so that the order is deterministic. While the effect of
> this can only been observed in a test tool, a subsequent commit will
> start to expose this functionality to users via a new `git reflog list`
> subcommand.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> refs/files-backend.c | 4 ++--
> t/t0600-reffiles-backend.sh | 4 ++--
> t/t1405-main-ref-store.sh | 2 +-
> t/t1406-submodule-ref-store.sh | 2 +-
> 4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 75dcc21ecb..2ffc63185f 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2193,7 +2193,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
>
> strbuf_addf(&sb, "%s/logs", gitdir);
>
> - diter = dir_iterator_begin(sb.buf, 0);
> + diter = dir_iterator_begin(sb.buf, DIR_ITERATOR_SORTED);
> if (!diter) {
> strbuf_release(&sb);
> return empty_ref_iterator_begin();
> @@ -2202,7 +2202,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
> CALLOC_ARRAY(iter, 1);
> ref_iterator = &iter->base;
>
> - base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);
> + base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1);
This caught my attention. Once we apply this patch, the only way
base_ref_iterator_init() can receive 0 for its last parameter
(i.e. 'ordered') is via the merge_ref_iterator_begin() call in
files_reflog_iterator_begin() that passes 0 as 'ordered'. If we
force files_reflog_iterator_begin() to ask for an ordered
merge_ref_iterator, then we will have no unordered ref iterators.
Am I reading the code right?
^ permalink raw reply
* Re: [PATCH 4/6] refs: drop unused params from the reflog iterator callback
From: Junio C Hamano @ 2024-02-20 0:14 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <be512ef268b910852ff11df181d89c483ffc18ab.1708353264.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> The ref and reflog iterators share much of the same underlying code to
> iterate over the corresponding entries. This results in some weird code
> because the reflog iterator also exposes an object ID as well as a flag
> to the callback function. Neither of these fields do refer to the reflog
> though -- they refer to the corresponding ref with the same name. This
> is quite misleading. In practice at least the object ID cannot really be
> implemented in any other way as a reflog does not have a specific object
> ID in the first place. This is further stressed by the fact that none of
> the callbacks except for our test helper make use of these fields.
Interesting observation. Of course this will make the callstack
longer by another level of indirection ...
> +struct do_for_each_reflog_help {
> + each_reflog_fn *fn;
> + void *cb_data;
> +};
> +
> +static int do_for_each_reflog_helper(struct repository *r UNUSED,
> + const char *refname,
> + const struct object_id *oid UNUSED,
> + int flags,
> + void *cb_data)
> +{
> + struct do_for_each_reflog_help *hp = cb_data;
> + return hp->fn(refname, hp->cb_data);
> +}
... but I think it would be worth it.
> +/*
> + * The signature for the callback function for the {refs_,}for_each_reflog()
> + * functions below. The memory pointed to by the refname argument is only
> + * guaranteed to be valid for the duration of a single callback invocation.
> + */
> +typedef int each_reflog_fn(const char *refname, void *cb_data);
> +
> /*
> * Calls the specified function for each reflog file until it returns nonzero,
> * and returns the value. Reflog file order is unspecified.
> */
> -int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data);
> -int for_each_reflog(each_ref_fn fn, void *cb_data);
> +int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data);
> +int for_each_reflog(each_reflog_fn fn, void *cb_data);
Nice simplification.
^ permalink raw reply
* Re: [PATCH 5/6] refs: stop resolving ref corresponding to reflogs
From: Junio C Hamano @ 2024-02-20 0:14 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <a7459b9483660d1a44df500aaee85ad38146eb02.1708353264.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Refactor the code to call `check_refname_format()` directly instead of
> trying to resolve the ref. This is significantly more efficient given
> that we don't have to hit the object database anymore to list reflogs.
> And second, it ensures that we end up showing reflogs of broken refs,
> which will help to make the reflog more useful.
And the user would notice corrupt ones among those reflogs listed
when using "rev-list -g" on the reflog anyway? Which sounds like a
sensible thing to do.
> Note that this really only impacts the case where the corresponding ref
> is corrupt. Reflogs for nonexistent refs would have been returned to the
> caller beforehand already as we did not pass `RESOLVE_REF_READING` to
> the function, and thus `refs_resolve_ref_unsafe()` would have returned
> successfully in that case.
What do "Reflogs for nonexistent refs" really mean? With the files
backend, if "git branch -d main" that removed the "main" branch
somehow forgot to remove the ".git/logs/refs/heads/main" file, the
reflog entries in such a file is for nonexistent ref. Is that what
you meant? As a tool to help diagnosing and correcting minor repo
breakages, finding such a leftover file that should not exist is a
good idea, I would think.
Would we see missing reflog for a ref that exists in the iteration?
I guess we shouldn't, as the reflog iterator that recursively
enumerates files under "$GIT_DIR/logs/" would not see such a missing
reflog by definition.
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 2b3c99b00d..741148087d 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2130,17 +2130,9 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
> while ((ok = dir_iterator_advance(diter)) == ITER_OK) {
> if (!S_ISREG(diter->st.st_mode))
> continue;
> - if (diter->basename[0] == '.')
> + if (check_refname_format(diter->basename,
> + REFNAME_ALLOW_ONELEVEL))
> continue;
A tangent.
I've never liked the code arrangement in the check_refname_format()
that assumes that each level can be separately checked with exactly
the same logic, and the only thing ALLOW_ONELEVEL does is to include
pseudorefs and HEAD; this makes such assumption even more ingrained.
I am not sure what to think about it, but let's keep reading.
> - if (ends_with(diter->basename, ".lock"))
> - continue;
This can safely go, as it is rejected by check_refname_format().
> - if (!refs_resolve_ref_unsafe(iter->ref_store,
> - diter->relative_path, 0,
> - NULL, NULL)) {
> - error("bad ref for %s", diter->path.buf);
> - continue;
> - }
This is the focus of this step in the series. We did not abort the
iteration before, but now we no longer issue any error message.
> iter->base.refname = diter->relative_path;
> return ITER_OK;
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 889bb1f1ba..efbbf23c72 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1659,11 +1659,9 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
> if (iter->last_name && !strcmp(iter->log.refname, iter->last_name))
> continue;
>
> - if (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,
> - 0, NULL, NULL)) {
> - error(_("bad ref for %s"), iter->log.refname);
> + if (check_refname_format(iter->log.refname,
> + REFNAME_ALLOW_ONELEVEL))
> continue;
> - }
This side is much more straight-forward. Looking good.
>
> free(iter->last_name);
> iter->last_name = xstrdup(iter->log.refname);
^ permalink raw reply
* Re: [GSoC] Use unsigned integral type for collection of bits
From: Junio C Hamano @ 2024-02-20 0:32 UTC (permalink / raw)
To: Eric Sunshine; +Cc: eugenio gigante, git
In-Reply-To: <CAPig+cR5K=pQjK+7ZUyGn1M50RZ0pRD6kOPQgmp7qez_LNXcAg@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
>> 'diff_filespec_is_binary' inside 'diff.c' would have to be changed.
>
> The code in question is not being used as a "bag of bits". Rather,
> it's a tristate binary with values "not-set", "true", and "false".
> Whereas a typical binary could be represented by a single bit, this
> one needs the extra bit to handle the "not-set" case. Moreover, it is
> idiomatic in the Git codebase for -1 to represent "not-set", so I
> think this code is fine as-is since its meaning is clear to those
> familiar with the codebase, thus does not need any changes made to it.
Correct. In general, bitfield structure members in our codebase
should be already fine. Most of them are "unsigned : 1" and there
is not enough room in a single-bit bitfield to go signed.
> There are cases in the codebase in which a signed type is being used
> as a "bag of bits" instead of the more desirable unsigned type.
> ...
> So, refresh_index() is correctly expecting an unsigned value for
> `flags` but refresh() in `builtin/add.c` has undesirably declared
> `flags` as signed.
Thanks for a good example. In a signed flag word that is used as a
bag of bits, the MSB is special, and unless you take advantage of
that special casing (which happens almost never), you should use an
unsigned word instead, to document that you are not doing anything
funny with the MSB, like "the flag word is negative, so the MSB must
be on", "I want to copy the bit immediately below MSB to MSB", etc.
^ permalink raw reply
* Re: [PATCH 6/6] builtin/reflog: introduce subcommand to list reflogs
From: Junio C Hamano @ 2024-02-20 0:32 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <cddb2de9394a07e405682e9ccdfdf5de92bb9092.1708353264.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index d2f5f42e67..6d8d5a253d 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -436,4 +436,73 @@ test_expect_success 'empty reflog' '
> test_must_be_empty err
> '
>
> +test_expect_success 'list reflogs' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> + git reflog list >actual &&
> + test_must_be_empty actual &&
> +
> + test_commit A &&
> + cat >expect <<-EOF &&
> + HEAD
> + refs/heads/main
> + EOF
> + git reflog list >actual &&
> + test_cmp expect actual &&
> +
> + git branch b &&
> + cat >expect <<-EOF &&
> + HEAD
> + refs/heads/b
> + refs/heads/main
> + EOF
> + git reflog list >actual &&
> + test_cmp expect actual
> + )
> +'
OK. This is a quite boring baseline.
> +test_expect_success 'reflog list returns error with additional args' '
> + cat >expect <<-EOF &&
> + error: list does not accept arguments: ${SQ}bogus${SQ}
> + EOF
> + test_must_fail git reflog list bogus 2>err &&
> + test_cmp expect err
> +'
Makes sense.
> +test_expect_success 'reflog for symref with unborn target can be listed' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> + test_commit A &&
> + git symbolic-ref HEAD refs/heads/unborn &&
> + cat >expect <<-EOF &&
> + HEAD
> + refs/heads/main
> + EOF
> + git reflog list >actual &&
> + test_cmp expect actual
> + )
> +'
Should this be under REFFILES? Ah, no, "git symbolic-ref" is valid
under reftable as well, so there is no need to.
Without [5/6], would it have failed to show the reflog for HEAD?
> +test_expect_success 'reflog with invalid object ID can be listed' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> + test_commit A &&
> + test-tool ref-store main update-ref msg refs/heads/missing \
> + $(test_oid deadbeef) "$ZERO_OID" REF_SKIP_OID_VERIFICATION &&
> + cat >expect <<-EOF &&
> + HEAD
> + refs/heads/main
> + refs/heads/missing
> + EOF
> + git reflog list >actual &&
> + test_cmp expect actual
> + )
> +'
OK.
> test_done
It would have been "interesting" to see an example of "there is a
reflog but the underlying ref for it is missing" case, but I think
that falls into a minor repository corruption category, so lack of
such a test is also fine.
^ permalink raw reply
* Re: [PATCH] documentation: send-email: use camel case consistently
From: Junio C Hamano @ 2024-02-20 0:52 UTC (permalink / raw)
To: Dragan Simic; +Cc: git
In-Reply-To: <b0577267402f6177d8ba5646e12d7691437e6e8f.1708060779.git.dsimic@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> Correct a few random "sendemail.*" configuration parameter names in the
> documentation that, for some reason, didn't use camel case format.
Thanks.
> There's only one "Fixes" tag, while there should actually be a whole bunch
> of them to cover all the patches that introduced the configuration parameter
> names fixed by this patch. I think we're fine with just one.
I suspect that we are even better off without any. The only reason
to have them is if we plan to cherry-pick this patch down to a
separate maintenance track that the "culprit" was cherry-picked or
merged to, but we typically do not do so, and if we want to do so,
we'd need a much better coverage.
Anyway, checking the output of
$ git grep -n -e '^[a-z]*\.[a-z]*[A-Z][A-Z][a-zA-Z]*::' Documentation/config/
and comparing it with the output of
$ git grep -n -e '^[a-z]*\.[a-z]*[A-Z][a-zA-Z]*::' Documentation/config/
I think we should spell "SSL" (which is an acronym) full in capital,
and possibly do the same for "CC", too.
All the other updates in this patch looked sensible, but I wasn't
being particularly careful, so an extra set or two of eyes are
certainly appreciated in case I missed any.
Thanks.
^ permalink raw reply
* [PATCH] trailer: fix comment/cut-line regression with opts->no_divider
From: Jeff King @ 2024-02-20 1:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Philippe Blain, Linus Arver, Git mailing list
In-Reply-To: <xmqqfrxo9ty2.fsf@gitster.g>
On Mon, Feb 19, 2024 at 10:42:45AM -0800, Junio C Hamano wrote:
> Thanks, both, for finding a rather unfortunate regression. Perhaps
> it is worth delaying 2.44 final by a week or so to include a fix (or
> a revert if it comes to it).
Hmm, I had thought this was pre-2.44, but it was actually in the 2.43.x
maintenance series (so it is not a regression going from 2.43.2 to
2.44.0, but it is from 2.43.0 to 2.44.0).
Anyway, the fix is pretty simple, so I think it may be OK to apply it
for 2.44.0-rc2. Here it is (prepared on top of la/trailer-cleanups, so
it could also be merged down for a v2.43.3 if you want to).
-- >8 --
Subject: trailer: fix comment/cut-line regression with opts->no_divider
Commit 97e9d0b78a (trailer: find the end of the log message, 2023-10-20)
combined two code paths for finding the end of the log message. For the
"no_divider" case, we used to use find_trailer_end(), and that has now
been rolled into find_end_of_log_message(). But there's a regression;
that function returns early when no_divider is set, returning the whole
string.
That's not how find_trailer_end() behaved. Although it did skip the
"---" processing (which is what "no_divider" is meant to do), we should
still respect ignored_log_message_bytes(), which covers things like
comments, "commit -v" cut lines, and so on.
The bug is actually in the interpret-trailers command, but the obvious
way to experience it is by running "commit -v" with a "--trailer"
option. The new trailer will be added at the end of the verbose diff,
rather than before it (and consequently will be ignored entirely, since
everything after the diff's intro scissors line is thrown away).
I've added two tests here: one for interpret-trailers directly, which
shows the bug via the parsing routines, and one for "commit -v".
The fix itself is pretty simple: instead of returning early, no_divider
just skips the "---" handling but still calls ignored_log_message_bytes().
Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Viewing the diff with "-w" makes the change a little more obvious.
t/t7502-commit-porcelain.sh | 18 ++++++++++++++++++
t/t7513-interpret-trailers.sh | 19 +++++++++++++++++++
trailer.c | 15 +++++++--------
3 files changed, 44 insertions(+), 8 deletions(-)
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index b5bf7de7cd..d8e216613f 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -485,6 +485,24 @@ test_expect_success 'commit --trailer not confused by --- separator' '
test_cmp expected actual
'
+test_expect_success 'commit --trailer with --verbose' '
+ cat >msg <<-\EOF &&
+ subject
+
+ body
+ EOF
+ GIT_EDITOR=: git commit --edit -F msg --allow-empty \
+ --trailer="my-trailer: value" --verbose &&
+ {
+ cat msg &&
+ echo &&
+ echo "my-trailer: value"
+ } >expected &&
+ git cat-file commit HEAD >commit.msg &&
+ sed -e "1,/^\$/d" commit.msg >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'multiple -m' '
>negative &&
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 97f10905d2..3343ad0eb6 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1560,4 +1560,23 @@ test_expect_success 'suppress --- handling' '
test_cmp expected actual
'
+test_expect_success 'suppressing --- does not disable cut-line handling' '
+ echo "real-trailer: before the cut" >expected &&
+
+ git interpret-trailers --parse --no-divider >actual <<-\EOF &&
+ subject
+
+ This input has a cut-line in it; we should stop parsing when we see it
+ and consider only trailers before that line.
+
+ real-trailer: before the cut
+
+ # ------------------------ >8 ------------------------
+ # Nothing below this line counts as part of the commit message.
+ not-a-trailer: too late
+ EOF
+
+ test_cmp expected actual
+'
+
test_done
diff --git a/trailer.c b/trailer.c
index 816f8b28a9..009ee80dee 100644
--- a/trailer.c
+++ b/trailer.c
@@ -837,16 +837,15 @@ static size_t find_end_of_log_message(const char *input, int no_divider)
/* Assume the naive end of the input is already what we want. */
end = strlen(input);
- if (no_divider)
- return end;
-
/* Optionally skip over any patch part ("---" line and below). */
- for (s = input; *s; s = next_line(s)) {
- const char *v;
+ if (!no_divider) {
+ for (s = input; *s; s = next_line(s)) {
+ const char *v;
- if (skip_prefix(s, "---", &v) && isspace(*v)) {
- end = s - input;
- break;
+ if (skip_prefix(s, "---", &v) && isspace(*v)) {
+ end = s - input;
+ break;
+ }
}
}
--
2.44.0.rc1.413.gdc9df0ba3d
^ permalink raw reply related
* Re: [PATCH v2] rerere: fix crash during clear
From: Junio C Hamano @ 2024-02-20 1:22 UTC (permalink / raw)
To: Marcel Röthke; +Cc: git
In-Reply-To: <20240218194603.1210895-1-marcel@roethke.info>
Marcel Röthke <marcel@roethke.info> writes:
> When rerere_clear is called, for instance when aborting a rebase, and
> the current conflict does not have a pre or postimage recorded git
> crashes with a SEGFAULT in has_rerere_resolution when accessing the
> status member of struct rerere_dir.
I had to read this twice before realizing the reason why I found it
hard to grok was because of a missing comma between "recorded" and
"git".
> This happens because scan_rerere_dir
> only allocates the status field in struct rerere_dir when a post or
> preimage was found.
But that is not really the root cause, no? Readers following the
above text are probably wondering why the preimage was not recorded,
when a conflict resulted in stopping a mergy-command and invoking
rerere machinery, before rerere_clear() got called. Is that
something that usually happen? How? Do we have a reproduction
sequence of such a state that we can make it into a new test in
t4200 where we already have tests for "git rerere clear" and its
friends?
> In some cases a segfault may happen even if a post
> or preimage was recorded if it was not for the variant of interest and
> the number of the variant that is present is lower than the variant of
> interest.
Ditto. What sequence of events would lead to such a state?
The answer *can* be ".git/rr-cache being a normal directory, the
user can poke around, removing files randomly, which can create such
a problematic situation", and the reproduction test *can* also be to
simulate such an end-user action, but I am asking primarily because
I want to make sure that we are *not* losing or failing to create
necessary preimage files, causing this problem to users who do not
muck with files in .git/rr-cache themselves.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/5] log: Move show_blob_object() to log.c
From: Junio C Hamano @ 2024-02-20 1:22 UTC (permalink / raw)
To: Maarten Bosmans; +Cc: git, Maarten Bosmans
In-Reply-To: <20240218195938.6253-2-maarten.bosmans@vortech.nl>
Maarten Bosmans <mkbosmans@gmail.com> writes:
> Subject: Re: [PATCH v2 1/5] log: Move show_blob_object() to log.c
"Move" -> "move".
> From: Maarten Bosmans <mkbosmans@gmail.com>
>
> From: Maarten Bosmans <maarten.bosmans@vortech.nl>
The earlier one is not needed. Please do not include such a line.
>
> This way it can be used outside of builtin/log.c.
> The next commit will make builtin/notes.c use it.
The resulting file is only about a small part of the implementation
detail of "show", and has very little to do with "log".
"show.c" that happens to house show_blob_object(), a "canonical" way
to display a blob object, with an anticipation that somebody may
want to expand it in the future to house the "canonical" way to
display a tag, a tree or a commit object, would be OK, though.
^ permalink raw reply
* Re: Why does the includeif woks how it does?
From: Junio C Hamano @ 2024-02-20 1:42 UTC (permalink / raw)
To: rsbecker; +Cc: 'brian m. carlson', 'Dominik von Haller', git
In-Reply-To: <01d401da6379$635fdb30$2a1f9190$@nexbridge.com>
<rsbecker@nexbridge.com> writes:
> I have considered contributing an "includewhere" option that would
> do that and differentiate from "includeif". I'm not sure it is
> required, and what would happen with symbolic links.
Other potential gotchas would include how it interacts with
directory hierarchies. You may be inside a directory /a/b (where
none of /, /a, and /a/b is controlled by git) and want your "git
init" invocation to be affected by some configuration included into
your $HOME/.gitconfig via include-where mechanism. Would it work
recursively? In other words, if you had
[includeIf "gitdir:/a/b"] path = $HOME/gits/a-b-in-repo
[includeIf "cwd:/"] path = $HOME/gits/others
[includeIf "cwd:/a"] path = $HOME/gits/a
[includeIf "cwd:/a/b"] path = $HOME/gits/a-b
would all of them be included? Just the last one? Does the most
specific one win?
After your "git init" succeeds, the one specified with gitdir: would
start kicking in. Would the "cwd:" ones that are meant for cases
outside any directory under Git control be ignored then?
I am not opposed to such a feature existing at all; just pointing
out there are sources of end-user confusion we need to be careful
while we design the feature.
^ permalink raw reply
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Jeff King @ 2024-02-20 1:51 UTC (permalink / raw)
To: Maarten Bosmans; +Cc: Junio C Hamano, git, Teng Long
In-Reply-To: <CA+CvcKSQCUukfLNnRkmTp=K=aXBRaxQnattfL+QexgOsYX18nA@mail.gmail.com>
On Sat, Feb 17, 2024 at 01:45:39PM +0100, Maarten Bosmans wrote:
> > Of the two, I'd guess that the second one is a lot less work to
> > implement (on the Git side; on the reading side it's a little more
> > involved, but still should be a constant number of processes).
>
> The second one is attractive for another reason than implementation
> simplicity. While the first one offers more flexibility, the second
> reuses the existing cat-file batch format, so the interface between
> git and scripts is familiar and consistent.
Agreed.
> > One variant of the second one is to use "git notes list". For example,
> > you can get all notes via cat-file like this right now:
> >
> > git notes list |
> > git cat-file --batch='%(objectname) %(objectsize) %(rest)'
>
> So the cat-file batch output is suitable for blobs containing newline
> or NUL characters. But I struggle a bit with what would be an easy way
> of using this format in a shell script. Something with multiple read
> and read -N commands reading from the output, I guess.
> The git codebase has `extract_batch_output()` in t1006. This uses a
> separate perl invocation to parse the cat-file output, which confirms
> my suspicion there isn't a straight-forward way to do this in e.g.
> just a bash script.
Yes, you could perhaps do it with "read -N". But note that it is a
bash-ism, and other POSIX shells may not support it. That's one of the
reasons we do not use it in t1006.
Also, I suspect that even bash does not retain NUL bytes in shell
variables. E.g.:
$ printf '\0foo\0bar\0' | cat -A
^@foo^@bar^@
$ printf '\0foo\0bar\0' | (read -r -N 9 var; echo "var=$var" | cat -A)
var=foobar$
So pure shell is probably a losing battle if you want to be binary
clean. You might be able to do something like using "read" to get the
header line, and then another tool like "head -c" to read those bytes.
But now you're back to one process per object. Plus depending on the
implementation of "head", it might or might not read more bytes from the
pipe as part of its stdio buffering.
So really, you are probably better off handling the output with a more
capable language (though again, there's not much Git can do here; the
complications are from handling binary data, and not Git's output format
choices).
> That was why my first steps were to accept that a launching a separate
> process per note in a bash loop is a pretty clear and well understood
> idiom in shell scripts and try to make the git part of that a bit more
> efficient.
Yes, I agree it's a simple idiom. And I certainly don't mind any
speedups we can get there, if they don't come with a cost (and your
patches looked pretty reasonable to me, though I didn't read them too
carefully). But I do think anytime you are invoking N+1 processes in a
shell script, it's kind of a losing battle for performance. :)
-Peff
^ permalink raw reply
* Re: [PATCH v2 1/5] log: Move show_blob_object() to log.c
From: Jeff King @ 2024-02-20 1:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Maarten Bosmans, git, Maarten Bosmans
In-Reply-To: <xmqqjzn0x72z.fsf@gitster.g>
On Mon, Feb 19, 2024 at 05:22:44PM -0800, Junio C Hamano wrote:
> > This way it can be used outside of builtin/log.c.
> > The next commit will make builtin/notes.c use it.
>
> The resulting file is only about a small part of the implementation
> detail of "show", and has very little to do with "log".
>
> "show.c" that happens to house show_blob_object(), a "canonical" way
> to display a blob object, with an anticipation that somebody may
> want to expand it in the future to house the "canonical" way to
> display a tag, a tree or a commit object, would be OK, though.
I'm not sure this function (or its siblings in builtin/log.c) counts as
"canonical", since it is deeply connected to "struct rev_info". So it is
appropriate for log, show, etc, but not for other commands.
It felt a little funny to me to make a new file just for this one
function. The related functions are all in log-tree.c. And as a bonus,
the name is _already_ horribly confusing because it says "tree" but the
functions are really about showing commits. ;)
All that said, I'm not sure based on our previous discussion why we
can't just call stream_blob_to_fd(). Looking at show_blob_object(), most
of the logic is about recording the tree-context of the given name and
using it for textconv. But since we know we are feeding a bare oid,
that would never kick in. So I don't know if there's any value in
sharing this function more widely in the first place.
-Peff
^ permalink raw reply
* Re: [PATCH v2 5/5] notes: use strbuf_attach to take ownership of the object contents
From: Jeff King @ 2024-02-20 2:12 UTC (permalink / raw)
To: Maarten Bosmans; +Cc: git, Maarten Bosmans
In-Reply-To: <20240218195938.6253-6-maarten.bosmans@vortech.nl>
On Sun, Feb 18, 2024 at 08:59:38PM +0100, Maarten Bosmans wrote:
> @@ -705,12 +703,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> if (!prev_buf)
> die(_("unable to read %s"), oid_to_hex(note));
> if (size)
> - strbuf_add(&buf, prev_buf, size);
> + strbuf_attach(&buf, prev_buf, size, size + 1);
> if (d.buf.len && size)
> append_separator(&buf);
> strbuf_insert(&d.buf, 0, buf.buf, buf.len);
>
> - free(prev_buf);
> strbuf_release(&buf);
> }
Is it possible for "size" to be 0, but prev_buf to be non-NULL? I assume
it is so if the previous note is the empty object (and anyway, we'd have
died earlier if prev_buf was NULL). In that case your patch introduces a
leak (we do not attach prev_buf to buf, but we no longer free prev_buf).
I'm a little skeptical that this is actually increasing the speed of the
command in a measurable way, though. It's one allocation/copy, right
next to a big old strbuf_insert() that is going to splice into an
existing array.
-Peff
^ permalink raw reply
* Re: Git commit causes data download in partial clone
From: Jeff King @ 2024-02-20 2:43 UTC (permalink / raw)
To: charmocc; +Cc: git@vger.kernel.org
In-Reply-To: <OK9E_kNDYqB1tDn6YJhtTgkdDDrcr2LhZEGRdmqismu6KyTki-M22CpCAxXHZCn45SZICjnPNYxvw02BnWeJic3mx47-zeI0HDhzdgoJpG0=@proton.me>
On Sat, Feb 17, 2024 at 08:38:08PM +0000, charmocc wrote:
> I was recently exploring git partial clone feature because I wanted to
> contribute to repository which has a lot of binary files. My intent was to only
> add new files without modifying any existing ones and to download as few data
> as possible in the process. Here are the steps I followed:
>
> $ git clone --no-checkout --filter=blob:none https://github.com/libretro-thumbnails/Nintendo_-_Nintendo_Entertainment_System.git nes
> $ cd nes
> $ echo foo > bar
> $ git add bar
> $ git commit bar # causes git fetch behind the scene and download of a lot of objects!
>
> Now for reasons I don't understand the last command cause download of a lot of
> objects from remote (blobs) which is what I was trying to avoid. By enabling
> tracing options I can see that it runs fetch operation in the background:
I think what is happening is something like:
1. You clone with --no-checkout, so you do not fetch any of the blobs.
But you also have an empty index, with no entries at all.
2. Running "git commit" is going to need all of those entries in the
index (to compute the hash of the new tree). So it will read it
from the tree of the current HEAD.
3. When we load entries into the index, the usual next thing to do is
to check them out. So rather than fetch them one by one as we do
the actual checkout, the index-reading code collects all of the
entries we don't have and then does a single fetch for them. This
is prefetch_cache_entries() in read-cache.c.
Now obviously in your example, the "usual" thing is not happening; we do
not intend to write those entries into the working tree, so fetching
them is pointless.
There may be some room for improvement here. E.g., teaching the
index-reading code a flag that says "don't bother prefetching", and use
it in this call chain. I'm not sure if there would be other gotchas,
though.
But here are a few alternatives that you can try without making any code
changes:
a. Your --no-checkout skips the checkout, but it does not tell Git
that you are fundamentally uninterested in those other paths. To do
that, you can try the sparse-checkout mechanism. I'm not super
familiar with the feature myself, but doing:
git clone --sparse --filter=blob:none $url nes
ends up with an empty checkout to which you can add things (the
trick is that we do have all of those index entries, but they are
marked as "not interesting").
Do note that --sparse checks out the contents of the top-level tree
by default. That's OK for your repo (all of the files are in the
Named_Titles directory), but it might not be true for some other
repos (it may also not work if your intent is to put another entry
into Named_Titles, though it looks like you might just need to say
"git add --sparse").
b. Skip the index entirely and just construct your own tree/commit.
E.g., doing:
blob=$(git hash-object -w some-file)
tree=$({
git ls-tree HEAD &&
printf "100644 blob $blob\t%s" some-file
} | git mktree --missing)
commit=$(echo my commit message | git commit-tree -p HEAD $tree)
git update-ref HEAD $commit
It gets a little trickier if your want to add to a sub-directory
(you have to recursively generate each tree).
In both cases you might also want to clone with "--depth 1", so you do
not bother grabbing old commits and trees, either.
> git version 2.34.1 (Ubuntu 22.04)
The sparse-checkout feature is new-ish and has been actively worked on
in the past few years. What I showed above works with the latest release
of Git, but you may or may not need to upgrade (I didn't dig into the
details).
-Peff
^ permalink raw reply
* Re: [PATCH v4] mergetools: vimdiff: use correct tool's name when reading mergetool config
From: Junio C Hamano @ 2024-02-20 2:52 UTC (permalink / raw)
To: Kipras Melnikovas; +Cc: git, greenfoo
In-Reply-To: <20240217162718.21272-1-kipras@kipras.org>
Kipras Melnikovas <kipras@kipras.org> writes:
> Okay I've finalised the documentation, should be the last patch.
> Also I realise I've forgotten to cc the mailing list on my replies to
> Junio and Fernando - sorry! First time..
>
> Range-diff against v3:
> 1: 0018c7e18c = 1: 0018c7e18c mergetools: vimdiff: use correct tool's name when reading mergetool config
>
> Documentation/config/mergetool.txt | 21 ++++++++++++++-------
> Documentation/mergetools/vimdiff.txt | 3 ++-
> mergetools/vimdiff | 12 ++++++++++--
> 3 files changed, 26 insertions(+), 10 deletions(-)
That's curious. This is v4 and no changes from v3?
^ permalink raw reply
* Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked
From: Junio C Hamano @ 2024-02-20 2:52 UTC (permalink / raw)
To: Phillip Wood; +Cc: MithicSpirit, git
In-Reply-To: <99346639-5a36-4c2e-a5d7-035c3c1fda8b@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> How does "stash.keepIndex" interact with "git rebase --autostash" and
> "git merge --autostash"? I think both those commands expect a clean
> index after running "git stash". They could just override the config
> setting but it might get a bit confusing if some commands respect the
> config and others don't.
The "autostash" feature fundamentally should not respect such
configuration variables, as the reason why they exist is not to
create a stash of any unusual kind the user expresses preference to
by the configuration variable(s), but to clear the slate and bring
both the working tree and the index pristine with respect to HEAD.
Also, when able, they would automatically unstash after the main
operation to rebase or merge is done, right? IOW, normally the use
of the stash ought to be invisible to the end users. I would not
worry too much about the "confusion" factor for these reasons.
You are however right that this will confuse the toolchain. These
two commands we provide may not be affected (or we can make them not
affected by changing their implementation if needed, while we add
such configuration variables at the same time), but third-party
tools and end-user scripts that has trusted that when they write
"git stash", the command will give them a clean index and working
tree will be broken big time.
So, I am somewhat negative on the patch in the current form, until I
see how the plan to help third-party tools and end-user scripts that
rely on the promise we have given them looks like.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/5] promise: introduce promises to track success or error
From: Jeff King @ 2024-02-20 2:57 UTC (permalink / raw)
To: phillip.wood
Cc: Philip Peterson via GitGitGadget, git, Johannes Schindelin,
Emily Shaffer, Philip Peterson
In-Reply-To: <bd340a27-bfb4-41b2-a1fa-356ab7dbbd36@gmail.com>
On Mon, Feb 19, 2024 at 02:25:29PM +0000, Phillip Wood wrote:
> I think we'd be better served by some kind of structured error type like the
> failure_result in this patch series that is allocated on the stack by the
> caller at the entry point to the library and passed down the call chain.
> That avoids the need for lots of dynamic allocations and allows us to
> continue allocating "out" parameters on the stack. For example
>
> int f(struct repository *r) {
> struct object_id oid;
>
> if (repo_get_oid(r, "HEAD", &oid))
> return error(_("could not parse HEAD"))
>
> /* use oid here */
> }
>
> would become
> int f(struct repository *r, struct error *err) {
> struct object_id oid;
>
> if (repo_get_oid(r, "HEAD", &oid))
> return error(&err, _("could not parse HEAD"))
>
> /* use oid here */
> }
>
> I'm sure this has been discussed in the past but I didn't manage to turn
> anything up with a quick search of the archive on lore.kernel.org.
There's some discussion in this sub-thread:
https://lore.kernel.org/git/20171103191309.sth4zjokgcupvk2e@sigill.intra.peff.net/
that also references this earlier thread:
https://lore.kernel.org/git/20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net/
I still think this is a reasonable way to go. At one point I had a
proof-of-concept conversion of some of the ref code, but I don't think I
have it any more.
-Peff
^ permalink raw reply
* Re: [PATCH v2 1/5] log: Move show_blob_object() to log.c
From: Junio C Hamano @ 2024-02-20 3:03 UTC (permalink / raw)
To: Jeff King; +Cc: Maarten Bosmans, git, Maarten Bosmans
In-Reply-To: <20240220015928.GB2713741@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> All that said, I'm not sure based on our previous discussion why we
> can't just call stream_blob_to_fd(). Looking at show_blob_object(), most
> of the logic is about recording the tree-context of the given name and
> using it for textconv. But since we know we are feeding a bare oid,
> that would never kick in. So I don't know if there's any value in
> sharing this function more widely in the first place.
It is very nice that we do not need to touch this code move at all.
Thanks, as usual, for a dose of sanity.
^ permalink raw reply
* Re: [PATCH] trailer: fix comment/cut-line regression with opts->no_divider
From: Junio C Hamano @ 2024-02-20 3:26 UTC (permalink / raw)
To: Jeff King; +Cc: Philippe Blain, Linus Arver, Git mailing list
In-Reply-To: <20240220010936.GA1793660@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Feb 19, 2024 at 10:42:45AM -0800, Junio C Hamano wrote:
>
>> Thanks, both, for finding a rather unfortunate regression. Perhaps
>> it is worth delaying 2.44 final by a week or so to include a fix (or
>> a revert if it comes to it).
>
> Hmm, I had thought this was pre-2.44, but it was actually in the 2.43.x
> maintenance series (so it is not a regression going from 2.43.2 to
> 2.44.0, but it is from 2.43.0 to 2.44.0).
I've been trying to be a bit aggressive this cycle to merge various
clean-up topics, together with real bugfixes, to 'maint'. Those who
often skip the -rcX but diligently follow numbered releases found a
few potential regressions in 2.44-rc as a result, which I could say
is a great success ;-). In addition, I was planning to have only
one -rc release without going -rc2 before the final, but we may need
one if only for this fix.
The fix in the patch looks quite straight-forward. Thanks.
^ permalink raw reply
* Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked
From: Ricardo C @ 2024-02-20 3:30 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood; +Cc: git
In-Reply-To: <xmqq34tnyhhf.fsf@gitster.g>
Hello Junio,
On 2/19/24 21:52, Junio C Hamano wrote:
> You are however right that this will confuse the toolchain. These
> two commands we provide may not be affected (or we can make them not
> affected by changing their implementation if needed, while we add
> such configuration variables at the same time), but third-party
> tools and end-user scripts that has trusted that when they write
> "git stash", the command will give them a clean index and working
> tree will be broken big time.
>
> So, I am somewhat negative on the patch in the current form, until I
> see how the plan to help third-party tools and end-user scripts that
> rely on the promise we have given them looks like.
This is an issue I hadn't considered, and I'm not sure whether it can even be
fixed. In some sense, the entire point of this patch is to allow the user to
break that promise in their configuration. However, I'm not sure how big of a
problem this is, as it is entirely opt-in (default behavior should be the same
as current behavior), and tools can be altered to pass `--no-keep-index
--no-include-untracked` if they wish to force the current behavior. Either
way, I would like to address your concern if possible, and I'd appreciate any
ideas on how to do so.
Thank you,
Ricardo
^ permalink raw reply
* Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked
From: Junio C Hamano @ 2024-02-20 3:44 UTC (permalink / raw)
To: Ricardo C; +Cc: Phillip Wood, git
In-Reply-To: <1d66eb0f-077a-4a63-8acf-f383538a41c7@gmail.com>
Ricardo C <rpc01234@gmail.com> writes:
> This is an issue I hadn't considered, and I'm not sure whether it can
> even be fixed. In some sense, the entire point of this patch is to
> allow the user to break that promise in their configuration. However,
> I'm not sure how big of a problem this is, as it is entirely opt-in
> (default behavior should be the same as current behavior),
Correct.
> and tools
> can be altered to pass `--no-keep-index --no-include-untracked` if
> they wish to force the current behavior.
This is not.
People expect a bit better from Git, and such a callous disregard to
backward compatibility that breaks other people's tools and scripts
is a non-starter.
Users of such tools, whether they were written by themselves or
other people, do *not* want them to break only because they want to
use a shiny new feature that is advertised in a new version of Git.
The point of packaging a solution, the reason why they wroute such a
tool or script that happens to use "git stash" as an ingredient and
depends on the current behaviour of "git stash", is so that they do
not need to remember they even used "git stash" as a small part of
their solution. And of course they do not want to remember that
they rely on how "git stash" behaves in such a solution. They do
not even want to bother complaining loudly when such a change is
proposed before it hits a release and hurt them. Saying "nobody
complained when these configuration variables were proposed" does
not help anybody later, after we already hurt them.
^ permalink raw reply
* Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked
From: Ricardo C @ 2024-02-20 3:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, git
In-Reply-To: <xmqq5xyjx0jc.fsf@gitster.g>
On 2/19/24 22:44, Junio C Hamano wrote:
> Ricardo C <rpc01234@gmail.com> writes:
>
>> This is an issue I hadn't considered, and I'm not sure whether it can
>> even be fixed. In some sense, the entire point of this patch is to
>> allow the user to break that promise in their configuration. However,
>> I'm not sure how big of a problem this is, as it is entirely opt-in
>> (default behavior should be the same as current behavior),
>
> Correct.
>
>> and tools
>> can be altered to pass `--no-keep-index --no-include-untracked` if
>> they wish to force the current behavior.
>
> This is not.
>
> People expect a bit better from Git, and such a callous disregard to
> backward compatibility that breaks other people's tools and scripts
> is a non-starter.
>
> Users of such tools, whether they were written by themselves or
> other people, do *not* want them to break only because they want to
> use a shiny new feature that is advertised in a new version of Git.
>
> The point of packaging a solution, the reason why they wroute such a
> tool or script that happens to use "git stash" as an ingredient and
> depends on the current behaviour of "git stash", is so that they do
> not need to remember they even used "git stash" as a small part of
> their solution. And of course they do not want to remember that
> they rely on how "git stash" behaves in such a solution. They do
> not even want to bother complaining loudly when such a change is
> proposed before it hits a release and hurt them. Saying "nobody
> complained when these configuration variables were proposed" does
> not help anybody later, after we already hurt them.
That makes sense. Do you have any ideas on how to address this? It feels to me
like providing this config option is fundamentally incompatible with requiring
backwards-compatible behavior regardless of configuration.
Ricardo
^ permalink raw reply
* Re: [PATCH] documentation: send-email: use camel case consistently
From: Dragan Simic @ 2024-02-20 6:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqv86kx8h0.fsf@gitster.g>
Hello Junio,
On 2024-02-20 01:52, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>> There's only one "Fixes" tag, while there should actually be a whole
>> bunch
>> of them to cover all the patches that introduced the configuration
>> parameter
>> names fixed by this patch. I think we're fine with just one.
>
> I suspect that we are even better off without any. The only reason
> to have them is if we plan to cherry-pick this patch down to a
> separate maintenance track that the "culprit" was cherry-picked or
> merged to, but we typically do not do so, and if we want to do so,
> we'd need a much better coverage.
Agreed, will drop the single "Fixes" tag in v2.
> Anyway, checking the output of
>
> $ git grep -n -e '^[a-z]*\.[a-z]*[A-Z][A-Z][a-zA-Z]*::'
> Documentation/config/
>
> and comparing it with the output of
>
> $ git grep -n -e '^[a-z]*\.[a-z]*[A-Z][a-zA-Z]*::'
> Documentation/config/
>
> I think we should spell "SSL" (which is an acronym) full in capital,
> and possibly do the same for "CC", too.
Agreed about "SSL", which I also though about doing that way initially;
will change in v2. There are already instances of "SSL" being used,
such as in various http.proxySSL* configuration parameter names.
Though, "CC" should remain written as "Cc", because it's the way email
headers are capitalized, which "Cc" refers to.
^ permalink raw reply
* Re: Why does the includeif woks how it does?
From: Dominik von Haller @ 2024-02-20 6:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: brian m. carlson, git@vger.kernel.org, rsbecker@nexbridge.com
Thanks, you all for your insights. <3
So to summarize, it was implemented through a gitdir because of possible confusions with the working directory. And there is no other implementation because the use case is not there.
For the new implementation I would always use the working directory. If set with git -C than use that. Should also not be a Problem because the working directory it is set internally anyway.
As far as the possible implementation of this goes, I would prefer an includeif with an extra option dir instead of gitdir. This would prevent confusion between includewhere and includeif.
The mentioned Problems seem to be easily figured out.
Every includeif == true will get included. Every more specific path will overwrite properties if set before. Gitdir is more specific than just dir.
Somewhat like that I would say is very intuitive.
Another possible approach which I would also say is logical is a top-down approach. Every Includeif would be executed from top to bottom. Every true includeif will be included and as before would overwrite properties if necessary.
Best regards
Dominik von Haller
^ 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