* Re: rebase invoking pre-commit
From: Junio C Hamano @ 2023-12-26 16:33 UTC (permalink / raw)
To: Phillip Wood; +Cc: Sean Allred, Git List
In-Reply-To: <bf1ce173-50d7-405f-88c1-7edb7ec5a55a@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> On 21/12/2023 20:58, Sean Allred wrote:
>> Is there a current reason why pre-commit shouldn't be invoked during
>> rebase, or is this just waiting for a reviewable patch?
>
> The reason that we don't run the pre-commit hook is that the commit
> being rebased may have been created with "git commit --no-verify" and
> so running the pre-commit hook would stop it from being rebased - see
> e637122ef2 (rebase -m: do not trigger pre-commit verification,
> 2008-03-16).
Very true. And back then we didn't have "rebase -x" mechanism but
these days, anybody who is interested in running a command between
each step can use it to run any validation script, not the one with
fixed name called "hooks", so I'd place this to fairly low priority.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Junio C Hamano @ 2023-12-26 17:01 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, ps, christian.couder
In-Reply-To: <CAOLa=ZRedfBUjukbN8dFT9CZETe4pz1UR+eWfJwORWuMHSk0Rw@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> Thanks for that, I did play around with trying to find files which
> could be refs in the $GIT_DIR, but the issue is that there will be
> false positives. e.g. `COMMIT_EDITMSG` could be confused for a
> pseudoref (passes is_pseudoref_syntax()) and it could potentially also
> contain a commit-ish value.
It would not begin with 40-hex, though. If I were doing this,
perhaps I'd say we should first split is_pseudoref_syntax() that is
overly loose into to classes (e.g. "caps with underscores that ends
with HEAD" and everything else), silently reject false positives
among the latter class. Then we rename those that are misnamed
(there should be only few, like AUTO_MERGE that should be a
pseudoref but named without _HEAD; I do not think of anything that
ends with _HEAD that is not a ref) over time and drop the latter
class.
> While you're here, I wonder if you have any thoughts on the last block
> of my first mail.
>
>> Over this, I'm also curious on what the mailing list thinks about
>> exposing this to the client side. Would an `--all` option for
>> git-for-each-ref(1) be sufficient?
"list pseudorefs in addition to things below refs/"? Sounds OK to
me as a feature.
However, "--all" does not mean that in the context of "git log"
family of commands. Over there, it means "not just --branches,
--tags, and --remotes, but everything" which is still limited below
"refs/".
As "git for-each-ref" takes pattern that is prefix match, e.g.,
$ git for-each-ref refs/remotes/
shows everything like refs/remotes/origin/main that begins with
refs/remotes/, I wonder if
$ git for-each-ref ""
should mean what you are asking for. After all, "git for-each-ref"
does *not* take "--branches" and others like "git log" family to
limit its operation to subhierarchy of "refs/" to begin with.
^ permalink raw reply
* Re: [PATCH] sideband.c: replace int with size_t for clarity
From: Junio C Hamano @ 2023-12-26 17:14 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1625.git.1703264469238.gitgitgadget@gmail.com>
"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> -static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
> +static void maybe_colorize_sideband(struct strbuf *dest, const char *src, size_t n)
Changing the type of the paramter alone might be a good start, but
does not really help anybody, as (1) the callers are not taught to
handle wider integral types for the values they pass and (2) the
function uses "int len" it computes internally to compare with "n".
There are three callers in demultiplex_sideband(), one of whose
paramters is "int len" and is passed to this function in one of
these calls. Among the other two, one uses "int linelen" derived
from scanning the piece of memory read from sideband via strpbrk(),
and the other uses strlen(b) which is the length of the "remainder"
of the same buffer after the loop that processes one line at a time
using the said strpbrk() is done with the complete lines in the
early part.
The buffer involved in all of the above stores bytes taken from a
packet received via the pkt-line interface, which is capable of
transferring only 64kB at a time.
I _think_ the most productive use of our time is to replace the
NEEDSWORK with a comment saying why it is fine to use "int" here to
avoid tempting the next developer to come up with this patch
again---they will waste their time to do so without thinking it
through if we leave the (incomplete) NEEDSWORK comment, I am afraid.
^ permalink raw reply
* Re: [PATCH] sparse-checkout: be consistent with end of options markers
From: Junio C Hamano @ 2023-12-26 17:18 UTC (permalink / raw)
To: Jeff King
Cc: Elijah Newren via GitGitGadget, git, Randall S. Becker,
Elijah Newren
In-Reply-To: <20231224083206.GA2053380@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sun, Dec 24, 2023 at 01:00:11AM +0000, Elijah Newren via GitGitGadget wrote:
>
>> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this
>> bug. Note that this does mean that anyone who might have been using
>> [...]
>
> Thanks for wrapping this up in patch form. It looks good to me,
> including the reasoning. You didn't add any tests, but I find it rather
> unlikely that we'd later regress here.
Surely. I am certainly OK with just dropping KEEP_UNKNOWN but I
would strongly prefer to document what we "fixed" (your "misspelt
option name" example) and what (mis|ab)use the people may have been
relying on we have "broken" (the same "misspelt" behaviour that can
be intentional is now forbidden, and we document that this change in
behaviour is intentional) with a new test.
Thanks.
^ permalink raw reply
* Re: [PATCH] sparse-checkout: be consistent with end of options markers
From: Junio C Hamano @ 2023-12-26 17:26 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget
Cc: git, Randall S. Becker, Jeff King, Elijah Newren
In-Reply-To: <pull.1625.git.git.1703379611749.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Elijah Newren <newren@gmail.com>
>
> 93851746 (parse-options: decouple "--end-of-options" and "--",
> 2023-12-06) updated the world order to make callers of parse-options
> that set PARSE_OPT_KEEP_UNKNOWN_OPT responsible for deciding what to
> do with "--end-of-options" they may see after parse_options() returns.
>
> This made a previous bug in sparse-checkout more visible; namely,
> that
>
> git sparse-checkout [add|set] --[no-]cone --end-of-options ...
>
> would simply treat "--end-of-options" as one of the paths to include in
> the sparse-checkout. But this was already problematic before; namely,
>
> git sparse-checkout [add|set| --[no-]cone --sikp-checks ...
>
> would not give an error on the mis-typed "--skip-checks" but instead
> simply treat "--sikp-checks" as a path or pattern to include in the
> sparse-checkout, which is highly unfriendly.
>
> This behavior begain when the command was converted to parse-options in
> 7bffca95ea (sparse-checkout: add '--stdin' option to set subcommand,
> 2019-11-21). Back then it was just called KEEP_UNKNOWN. Later it was
> renamed to KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options:
> PARSE_OPT_KEEP_UNKNOWN only applies to --options, 2022-08-19) to clarify
> that it was only about dashed options; we always keep non-option
> arguments. Looking at that original patch, both Peff and I think that
> the author was simply confused about the mis-named option, and really
> just wanted to keep the non-option arguments. We never should have used
> the flag all along (and the other cases were cargo-culted within the
> file).
>
> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this
> bug. Note that this does mean that anyone who might have been using
>
> git sparse-checkout [add|set] [--[no-]cone] --foo --bar
>
> to request paths or patterns '--foo' and '--bar' will now have to use
>
> git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar
>
> That makes sparse-checkout more consistent with other git commands,
> provides users much friendlier error messages and behavior, and is
> consistent with the all-capps warning in git-sparse-checkout.txt that
> this command "is experimental...its behavior...will likely change". :-)
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
Nicely described.
Apparently we do not have an existing test to cover the case of
"misspelt options becoming a pattern" that this bugfix corrects;
otherwise we would have a s/failure/success/ to update such an older
expectation, and have to wonder if the existing behaviour is
intentional. Since there is no such test, we can feel a bit safer
in "fixing" this odd behaviour.
Also, this corrects "--end-of-options" that becomes one of the
patterns. Such a bug was not detected in our test suite.
So we should at least have two new tests.
(1) Pass "--foo" without the end of options marker and make sure we
error out, instead of making it as one of the patterns.
(2) Pass "--end-of-options foo" and make sure the command succeeds,
an d"--end-of-options" does not become on eof the patterns.
Thanks.
^ permalink raw reply
* Re: [PATCH v3] Teach git apply to respect core.fileMode settings
From: Junio C Hamano @ 2023-12-26 17:35 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Chandra Pratap via GitGitGadget, git, Torsten Bögershausen,
Chandra Pratap, Chandra Pratap
In-Reply-To: <82dadb69-5016-dec6-3699-4d994ea7929d@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I noticed a CI breakage in t2106.3 in `seen` that seems to be caused by
> this, and I can make it go away with this patch:
>
> -- snip --
> From 5c2a709b629d396528dabe2f92bf3d4deb5bbdb2 Mon Sep 17 00:00:00 2001
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Date: Sun, 24 Dec 2023 14:01:49 +0100
> Subject: [PATCH] fixup! Teach git apply to respect core.fileMode settings
>
> As pointed out e.g. by t2016.3(git checkout -p), if the patch is to be
> applied in reverse (`git apply -R`), then the `old_mode` is actually 0,
> and we must use `new_mode` instead.
Good finding.
> While at it, add some defensive code to ignore `ce_mode` should it be 0.
Is it defensive or is it hiding a problematic index under the rug?
If there is an index entry whose ce_mode is 0, I suspect we would
want to error out with a BUG(), unless it is an intent-to-add entry.
Shouldn't it cause an error to apply a patch that mucks with
"newfile" after you did
$ git add -N newfile
If we allow ce_mode==0 to be propagated to st_mode, I suspect we
will catch such a case with the "mode is different" warning code, at
least.
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> apply.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/apply.c b/apply.c
> index 58f26c404136..5ad06ef2f843 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3780,7 +3780,9 @@ static int check_preimage(struct apply_state *state,
>
> if (!state->cached && !previous) {
> if (!trust_executable_bit)
> - st_mode = *ce ? (*ce)->ce_mode : patch->old_mode;
> + st_mode = *ce && (*ce)->ce_mode ? (*ce)->ce_mode :
> + (state->apply_in_reverse ?
> + patch->new_mode : patch->old_mode);
> else
> st_mode = ce_mode_from_stat(*ce, st->st_mode);
> }
> -- snap --
>
> I guess you can slap on that `Reviewed-by:` footer again, after all... ;-)
Yup, Reviewed-by: is what the reviewer says "this version was
reviewed by me and I found it satisfactory", so once you said the
above, I can certainly do so.
^ permalink raw reply
* Re: [PATCH 0/2] Fix doc placeholders
From: Junio C Hamano @ 2023-12-26 18:30 UTC (permalink / raw)
To: Jean-Noël Avila via GitGitGadget; +Cc: git, Jean-Noël Avila
In-Reply-To: <pull.1626.git.1703539287.gitgitgadget@gmail.com>
"Jean-Noël Avila via GitGitGadget" <gitgitgadget@gmail.com> writes:
> While setting up a check of (the absence of) translations of commands,
> options and environment variable in the manpages in the manpage translation
> project, some false positives appeared.
>
> Here is a series that enforces the formatting rules for placeholders in
> documentation and help.
Nice. Will take a look and then queue. Thanks.
>
> Jean-Noël Avila (2):
> doc: enforce dashes in placeholders
> doc: enforce placeholders in documentation
>
> Documentation/diff-options.txt | 2 +-
> Documentation/git-blame.txt | 2 +-
> Documentation/git-bugreport.txt | 4 ++--
> Documentation/git-commit-graph.txt | 2 +-
> Documentation/git-config.txt | 8 ++++----
> Documentation/git-cvsserver.txt | 4 ++--
> Documentation/git-daemon.txt | 10 +++++-----
> Documentation/git-diagnose.txt | 2 +-
> Documentation/git-difftool.txt | 2 +-
> Documentation/git-fast-import.txt | 4 ++--
> Documentation/git-fetch.txt | 4 ++--
> Documentation/git-filter-branch.txt | 6 +++---
> Documentation/git-format-patch.txt | 20 ++++++++++----------
> Documentation/git-mv.txt | 2 +-
> Documentation/git-notes.txt | 2 +-
> Documentation/git-replace.txt | 6 +++---
> Documentation/git-revert.txt | 4 ++--
> Documentation/git-send-email.txt | 2 +-
> Documentation/git-status.txt | 4 ++--
> Documentation/git-submodule.txt | 4 ++--
> Documentation/git-svn.txt | 18 +++++++++---------
> Documentation/git-tag.txt | 2 +-
> Documentation/git.txt | 4 ++--
> Documentation/gitdiffcore.txt | 8 ++++----
> Documentation/gitformat-index.txt | 4 ++--
> Documentation/githooks.txt | 8 ++++----
> Documentation/gitk.txt | 4 ++--
> Documentation/gitprotocol-capabilities.txt | 2 +-
> Documentation/gitprotocol-http.txt | 14 +++++++-------
> Documentation/gitprotocol-v2.txt | 8 ++++----
> Documentation/gitsubmodules.txt | 4 ++--
> Documentation/gitweb.conf.txt | 10 +++++-----
> Documentation/gitweb.txt | 2 +-
> Documentation/trace2-target-values.txt | 2 +-
> Documentation/urls.txt | 8 ++++----
> Documentation/user-manual.txt | 4 ++--
> builtin/commit-graph.c | 2 +-
> 37 files changed, 99 insertions(+), 99 deletions(-)
>
>
> base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1626%2Fjnavila%2Ffix_doc_placeholders-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1626/jnavila/fix_doc_placeholders-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1626
^ permalink raw reply
* Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Junio C Hamano @ 2023-12-26 18:45 UTC (permalink / raw)
To: Achu Luma; +Cc: christian.couder, git, Christian Couder
In-Reply-To: <20231221231527.8130-1-ach.lumap@gmail.com>
Achu Luma <ach.lumap@gmail.com> writes:
> 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;
> -}
So, if we have a is_foo() that characterises a byte that ought to
be "foo" but gets miscategorised not to be "foo", we used to
pinpoint exactly the byte value that was an issue. We did not do
any early return ...
> ...
> -#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); \
> -}
... and reported for all errors in the "class".
> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
> new file mode 100644
> index 0000000000..41189ba9f9
> --- /dev/null
> +++ b/t/unit-tests/t-ctype.c
> @@ -0,0 +1,76 @@
> +#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) \
> +{ \
> + int i; \
> + for (i = 0; i < 256; i++) \
> + check_int(func(i), ==, is_in(string, i)); \
> +}
Now, we let check_int() to do the checking for each and every byte
value for the class. check_int() uses different reporting and shows
the problematic value in a way that is more verbose and at the same
time is a less specific and harder to understand:
test_msg(" left: %"PRIdMAX, a);
test_msg(" right: %"PRIdMAX, b);
But that is probably the price to pay to use a more generic
framework, I guess.
> +int cmd_main(int argc, const char **argv) {
> + /* Run all character type tests */
> + TEST(test_ctype_isspace(), "isspace() works as we expect");
> + TEST(test_ctype_isdigit(), "isdigit() works as we expect");
> + TEST(test_ctype_isalpha(), "isalpha() works as we expect");
> + TEST(test_ctype_isalnum(), "isalnum() works as we expect");
> + TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect");
> + TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect");
> + TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect");
> + TEST(test_ctype_isascii(), "isascii() works as we expect");
> + TEST(test_ctype_islower(), "islower() works as we expect");
> + TEST(test_ctype_isupper(), "isupper() works as we expect");
> + TEST(test_ctype_iscntrl(), "iscntrl() works as we expect");
> + TEST(test_ctype_ispunct(), "ispunct() works as we expect");
> + TEST(test_ctype_isxdigit(), "isxdigit() works as we expect");
> + TEST(test_ctype_isprint(), "isprint() works as we expect");
> +
> + return test_done();
> +}
As a practice to use the unit-tests framework, the patch looks OK.
helper/test-ctype.c indeed is an oddball that runs once and checks
everything it wants to check, for which the unit tests framework is
much more suited.
Let's see how others react and then queue.
Thanks.
^ permalink raw reply
* Re: [PATCH v3] Teach git apply to respect core.fileMode settings
From: Johannes Schindelin @ 2023-12-26 19:18 UTC (permalink / raw)
To: Junio C Hamano
Cc: Chandra Pratap via GitGitGadget, git, Torsten Bögershausen,
Chandra Pratap, Chandra Pratap
In-Reply-To: <xmqq4jg4j28z.fsf@gitster.g>
Hi Junio,
On Tue, 26 Dec 2023, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > While at it, add some defensive code to ignore `ce_mode` should it be 0.
>
> Is it defensive or is it hiding a problematic index under the rug?
I wrote this defensive code only out of habit, not because I saw a
`ce_mode` that was 0.
> If there is an index entry whose ce_mode is 0, I suspect we would
> want to error out with a BUG(), unless it is an intent-to-add entry.
>
> Shouldn't it cause an error to apply a patch that mucks with
> "newfile" after you did
>
> $ git add -N newfile
>
> If we allow ce_mode==0 to be propagated to st_mode, I suspect we
> will catch such a case with the "mode is different" warning code, at
> least.
Is `ce_mode == 0` an indicator of a new file? In my tests, `git add -N`
will add the file with a non-zero mode...
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] sparse-checkout: be consistent with end of options markers
From: Elijah Newren @ 2023-12-26 19:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren via GitGitGadget, git, Randall S. Becker, Jeff King
In-Reply-To: <xmqq8r5gj2o3.fsf@gitster.g>
Hi,
On Tue, Dec 26, 2023 at 9:26 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > 93851746 (parse-options: decouple "--end-of-options" and "--",
> > 2023-12-06) updated the world order to make callers of parse-options
> > that set PARSE_OPT_KEEP_UNKNOWN_OPT responsible for deciding what to
> > do with "--end-of-options" they may see after parse_options() returns.
> >
> > This made a previous bug in sparse-checkout more visible; namely,
> > that
> >
> > git sparse-checkout [add|set] --[no-]cone --end-of-options ...
> >
> > would simply treat "--end-of-options" as one of the paths to include in
> > the sparse-checkout. But this was already problematic before; namely,
> >
> > git sparse-checkout [add|set| --[no-]cone --sikp-checks ...
> >
> > would not give an error on the mis-typed "--skip-checks" but instead
> > simply treat "--sikp-checks" as a path or pattern to include in the
> > sparse-checkout, which is highly unfriendly.
> >
> > This behavior begain when the command was converted to parse-options in
> > 7bffca95ea (sparse-checkout: add '--stdin' option to set subcommand,
> > 2019-11-21). Back then it was just called KEEP_UNKNOWN. Later it was
> > renamed to KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options:
> > PARSE_OPT_KEEP_UNKNOWN only applies to --options, 2022-08-19) to clarify
> > that it was only about dashed options; we always keep non-option
> > arguments. Looking at that original patch, both Peff and I think that
> > the author was simply confused about the mis-named option, and really
> > just wanted to keep the non-option arguments. We never should have used
> > the flag all along (and the other cases were cargo-culted within the
> > file).
> >
> > Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this
> > bug. Note that this does mean that anyone who might have been using
> >
> > git sparse-checkout [add|set] [--[no-]cone] --foo --bar
> >
> > to request paths or patterns '--foo' and '--bar' will now have to use
> >
> > git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar
> >
> > That makes sparse-checkout more consistent with other git commands,
> > provides users much friendlier error messages and behavior, and is
> > consistent with the all-capps warning in git-sparse-checkout.txt that
> > this command "is experimental...its behavior...will likely change". :-)
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
>
> Nicely described.
>
> Apparently we do not have an existing test to cover the case of
> "misspelt options becoming a pattern" that this bugfix corrects;
> otherwise we would have a s/failure/success/ to update such an older
> expectation, and have to wonder if the existing behaviour is
> intentional. Since there is no such test, we can feel a bit safer
> in "fixing" this odd behaviour.
>
> Also, this corrects "--end-of-options" that becomes one of the
> patterns. Such a bug was not detected in our test suite.
>
> So we should at least have two new tests.
>
> (1) Pass "--foo" without the end of options marker and make sure we
> error out, instead of making it as one of the patterns.
>
> (2) Pass "--end-of-options foo" and make sure the command succeeds,
> and "--end-of-options" does not become one of the patterns.
>
> Thanks.
I did actually create two such tests last Saturday, but they obviously
somehow went missing from my submission (I guess even if the high
fevers from Wed-Fri last week were gone, I was still more affected
than I realized?) Anyway, I'll resubmit with those test cases.
^ permalink raw reply
* Re: [PATCH] fast-import: use mem_pool_calloc()
From: Elijah Newren @ 2023-12-26 19:22 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <50c1f410-ca37-4c1c-a28b-3e9fad49f2b4@web.de>
On Tue, Dec 26, 2023 at 12:18 AM René Scharfe <l.s.r@web.de> wrote:
>
> Use mem_pool_calloc() to get a zeroed buffer instead of zeroing it
> ourselves. This makes the code clearer and less repetitive.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> builtin/fast-import.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 444f41cf8c..92eda20683 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -2809,8 +2809,7 @@ static void parse_new_tag(const char *arg)
> enum object_type type;
> const char *v;
>
> - t = mem_pool_alloc(&fi_mem_pool, sizeof(struct tag));
> - memset(t, 0, sizeof(struct tag));
> + t = mem_pool_calloc(&fi_mem_pool, 1, sizeof(struct tag));
> t->name = mem_pool_strdup(&fi_mem_pool, arg);
> if (last_tag)
> last_tag->next_tag = t;
> --
> 2.43.0
Makes sense; looks good to me.
^ permalink raw reply
* [PATCH v2] sparse-checkout: be consistent with end of options markers
From: Elijah Newren via GitGitGadget @ 2023-12-26 19:39 UTC (permalink / raw)
To: git
Cc: Randall S. Becker, Jeff King, Elijah Newren, Elijah Newren,
Elijah Newren
In-Reply-To: <pull.1625.git.git.1703379611749.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
93851746 (parse-options: decouple "--end-of-options" and "--",
2023-12-06) updated the world order to make callers of parse-options
that set PARSE_OPT_KEEP_UNKNOWN_OPT responsible for deciding what to
do with "--end-of-options" they may see after parse_options() returns.
This made a previous bug in sparse-checkout more visible; namely,
that
git sparse-checkout [add|set] --[no-]cone --end-of-options ...
would simply treat "--end-of-options" as one of the paths to include in
the sparse-checkout. But this was already problematic before; namely,
git sparse-checkout [add|set| --[no-]cone --sikp-checks ...
would not give an error on the mis-typed "--skip-checks" but instead
simply treat "--sikp-checks" as a path or pattern to include in the
sparse-checkout, which is highly unfriendly.
This behavior began when the command was converted to parse-options in
7bffca95ea (sparse-checkout: add '--stdin' option to set subcommand,
2019-11-21). Back then it was just called KEEP_UNKNOWN. Later it was
renamed to KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options:
PARSE_OPT_KEEP_UNKNOWN only applies to --options, 2022-08-19) to clarify
that it was only about dashed options; we always keep non-option
arguments. Looking at that original patch, both Peff and I think that
the author was simply confused about the mis-named option, and really
just wanted to keep the non-option arguments. We never should have used
the flag all along (and the other cases were cargo-culted within the
file).
Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this
bug. Note that this does mean that anyone who might have been using
git sparse-checkout [add|set] [--[no-]cone] --foo --bar
to request paths or patterns '--foo' and '--bar' will now have to use
git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar
That makes sparse-checkout more consistent with other git commands,
provides users much friendlier error messages and behavior, and is
consistent with the all-caps warning in git-sparse-checkout.txt that
this command "is experimental...its behavior...will likely change". :-)
Signed-off-by: Elijah Newren <newren@gmail.com>
---
[RFC] sparse-checkout: be consistent with end of options markers
Follow-up to thread over at
https://lore.kernel.org/git/CABPp-BF9XZeESHdxdcZ91Vsn5tKqQ6_3tC11e7t9vTFp=uufbg@mail.gmail.com/,
making end of options markers in git-sparse-checkout consistent with how
other git commands behave.
Changes since v1:
* Added some testcases
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1625%2Fgitgitgadget%2Fsparse-checkout-end-of-options-consistency-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1625/gitgitgadget/sparse-checkout-end-of-options-consistency-v2
Pull-Request: https://github.com/git/git/pull/1625
Range-diff vs v1:
1: c19156919a6 ! 1: 448146637a9 sparse-checkout: be consistent with end of options markers
@@ Commit message
simply treat "--sikp-checks" as a path or pattern to include in the
sparse-checkout, which is highly unfriendly.
- This behavior begain when the command was converted to parse-options in
+ This behavior began when the command was converted to parse-options in
7bffca95ea (sparse-checkout: add '--stdin' option to set subcommand,
2019-11-21). Back then it was just called KEEP_UNKNOWN. Later it was
renamed to KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options:
@@ Commit message
That makes sparse-checkout more consistent with other git commands,
provides users much friendlier error messages and behavior, and is
- consistent with the all-capps warning in git-sparse-checkout.txt that
+ consistent with the all-caps warning in git-sparse-checkout.txt that
this command "is experimental...its behavior...will likely change". :-)
Signed-off-by: Elijah Newren <newren@gmail.com>
@@ builtin/sparse-checkout.c: static int sparse_checkout_check_rules(int argc, cons
if (check_rules_opts.rules_file && check_rules_opts.cone_mode < 0)
check_rules_opts.cone_mode = 1;
+
+ ## t/t1091-sparse-checkout-builtin.sh ##
+@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'cone mode: set with nested folders' '
+
+ test_expect_success 'cone mode: add independent path' '
+ git -C repo sparse-checkout set deep/deeper1 &&
+- git -C repo sparse-checkout add folder1 &&
++ git -C repo sparse-checkout add --end-of-options folder1 &&
+ cat >expect <<-\EOF &&
+ /*
+ !/*/
+@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'by default, cone mode will error out when passed files' '
+ grep ".gitignore.*is not a directory" error
+ '
+
++test_expect_success 'error on mistyped command line options' '
++ test_must_fail git -C repo sparse-checkout add --sikp-checks .gitignore 2>error &&
++
++ grep "unknown option.*sikp-checks" error
++'
++
+ test_expect_success 'by default, non-cone mode will warn on individual files' '
+ git -C repo sparse-checkout reapply --no-cone &&
+ git -C repo sparse-checkout add .gitignore 2>warning &&
builtin/sparse-checkout.c | 9 +++------
t/t1091-sparse-checkout-builtin.sh | 8 +++++++-
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 5c8ffb1f759..0e68e9b0b0d 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -777,8 +777,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix,
builtin_sparse_checkout_add_options,
- builtin_sparse_checkout_add_usage,
- PARSE_OPT_KEEP_UNKNOWN_OPT);
+ builtin_sparse_checkout_add_usage, 0);
sanitize_paths(argc, argv, prefix, add_opts.skip_checks);
@@ -824,8 +823,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix,
builtin_sparse_checkout_set_options,
- builtin_sparse_checkout_set_usage,
- PARSE_OPT_KEEP_UNKNOWN_OPT);
+ builtin_sparse_checkout_set_usage, 0);
if (update_modes(&set_opts.cone_mode, &set_opts.sparse_index))
return 1;
@@ -996,8 +994,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char *
argc = parse_options(argc, argv, prefix,
builtin_sparse_checkout_check_rules_options,
- builtin_sparse_checkout_check_rules_usage,
- PARSE_OPT_KEEP_UNKNOWN_OPT);
+ builtin_sparse_checkout_check_rules_usage, 0);
if (check_rules_opts.rules_file && check_rules_opts.cone_mode < 0)
check_rules_opts.cone_mode = 1;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index f67611da28e..e49b8024ac5 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -334,7 +334,7 @@ test_expect_success 'cone mode: set with nested folders' '
test_expect_success 'cone mode: add independent path' '
git -C repo sparse-checkout set deep/deeper1 &&
- git -C repo sparse-checkout add folder1 &&
+ git -C repo sparse-checkout add --end-of-options folder1 &&
cat >expect <<-\EOF &&
/*
!/*/
@@ -886,6 +886,12 @@ test_expect_success 'by default, cone mode will error out when passed files' '
grep ".gitignore.*is not a directory" error
'
+test_expect_success 'error on mistyped command line options' '
+ test_must_fail git -C repo sparse-checkout add --sikp-checks .gitignore 2>error &&
+
+ grep "unknown option.*sikp-checks" error
+'
+
test_expect_success 'by default, non-cone mode will warn on individual files' '
git -C repo sparse-checkout reapply --no-cone &&
git -C repo sparse-checkout add .gitignore 2>warning &&
base-commit: 055bb6e9969085777b7fab83e3fee0017654f134
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v3] Teach git apply to respect core.fileMode settings
From: Junio C Hamano @ 2023-12-26 20:10 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Chandra Pratap via GitGitGadget, git, Torsten Bögershausen,
Chandra Pratap, Chandra Pratap
In-Reply-To: <fb022e3c-adb5-e341-9fd0-9c5311abe908@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Is it defensive or is it hiding a problematic index under the rug?
>
> I wrote this defensive code only out of habit, not because I saw a
> `ce_mode` that was 0.
>
>> If there is an index entry whose ce_mode is 0, I suspect we would
>> want to error out with a BUG(), unless it is an intent-to-add entry.
>>
>> Shouldn't it cause an error to apply a patch that mucks with
>> "newfile" after you did
>>
>> $ git add -N newfile
>>
>> If we allow ce_mode==0 to be propagated to st_mode, I suspect we
>> will catch such a case with the "mode is different" warning code, at
>> least.
>
> Is `ce_mode == 0` an indicator of a new file? In my tests, `git add -N`
> will add the file with a non-zero mode...
Oh, if we know nobody would assign 0 to ce_mode in a valid in-index
entry, then we should (1) check and BUG() if we care there may be
such a case due to a bug, or (2) assume that it never happens and
omit the extra check. The third way in the patch is neither and is
sweeping a potential bug ("potential" because the code apparently
assumes it can happen) under the rug ("sweeping" because the code
silently ignores such an abnormal case), I am afraid.
^ permalink raw reply
* Re: [PATCH v2] sparse-checkout: be consistent with end of options markers
From: Junio C Hamano @ 2023-12-26 20:12 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget
Cc: git, Randall S. Becker, Jeff King, Elijah Newren
In-Reply-To: <pull.1625.v2.git.git.1703619562639.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this
> bug. Note that this does mean that anyone who might have been using
>
> git sparse-checkout [add|set] [--[no-]cone] --foo --bar
>
> to request paths or patterns '--foo' and '--bar' will now have to use
>
> git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar
>
> That makes sparse-checkout more consistent with other git commands,
> provides users much friendlier error messages and behavior, and is
> consistent with the all-caps warning in git-sparse-checkout.txt that
> this command "is experimental...its behavior...will likely change". :-)
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
Let me drop jc/sparse-checkout-eoo topic and queue this instead, and
then resurrect the "use default for 'set' only when !stdin" as a
separate topic.
Thanks.
^ permalink raw reply
* [PATCH v2] diff: implement config.diff.renames=copies-harder
From: Sam James @ 2023-12-26 20:20 UTC (permalink / raw)
To: git; +Cc: Sam James, Elijah Newren, Junio C Hamano
This patch adds a config value for 'diff.renames' called 'copies-harder'
which make it so '-C -C' is in effect always passed for 'git log -p',
'git diff', etc.
This allows specifying that 'git log -p', 'git diff', etc should always act
as if '-C --find-copies-harder' was passed.
It has proven this especially useful for certain types of repository (like
Gentoo's ebuild repositories) because files are often copies of a previous
version:
Suppose a directory 'sys-devel/gcc' contains recipes for building
GCC, with one file for each supported upstream branch:
gcc-13.x.build.recipe
gcc-12.x.build.recipe
gcc-11.x.build.recipe
gcc-10.x.build.recipe
gcc-13.x.build.recipe was started as a copy of gcc-12.x.build.recipe
(which was started as a copy of gcc-11.x.build.recipe, etc.). Previous versions
are kept around to support parallel installation of multiple versions.
Being able to easily observe the diff relative to other recipes within the
directory has been a quality of life improvement for such repo layouts.
Cc: Elijah Newren <newren@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Sam James <sam@gentoo.org>
---
v2: Improve the commit message using Elijah Newren's example (it is indeed
precisely what I was thinking of, but phrased better), and improve the documentation
to explain better what the new config option actually does & refer to git-diff's
--find-copies-harder.
Documentation/config/diff.txt | 8 +++++---
Documentation/config/status.txt | 3 ++-
diff.c | 12 +++++++++---
diff.h | 1 +
diffcore-rename.c | 4 ++--
merge-ort.c | 2 +-
merge-recursive.c | 2 +-
7 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
index bd5ae0c337..cdd8a74ec0 100644
--- a/Documentation/config/diff.txt
+++ b/Documentation/config/diff.txt
@@ -131,9 +131,11 @@ diff.renames::
Whether and how Git detects renames. If set to "false",
rename detection is disabled. If set to "true", basic rename
detection is enabled. If set to "copies" or "copy", Git will
- detect copies, as well. Defaults to true. Note that this
- affects only 'git diff' Porcelain like linkgit:git-diff[1] and
- linkgit:git-log[1], and not lower level commands such as
+ detect copies, as well. If set to "copies-harder", Git will spend extra
+ cycles to find more copies even in unmodified paths, see
+ '--find-copies-harder' in linkgit:git-diff[1]. Defaults to true.
+ Note that this affects only 'git diff' Porcelain like linkgit:git-diff[1]
+ and linkgit:git-log[1], and not lower level commands such as
linkgit:git-diff-files[1].
diff.suppressBlankEmpty::
diff --git a/Documentation/config/status.txt b/Documentation/config/status.txt
index 2ff8237f8f..7ca7a4becd 100644
--- a/Documentation/config/status.txt
+++ b/Documentation/config/status.txt
@@ -33,7 +33,8 @@ status.renames::
Whether and how Git detects renames in linkgit:git-status[1] and
linkgit:git-commit[1] . If set to "false", rename detection is
disabled. If set to "true", basic rename detection is enabled.
- If set to "copies" or "copy", Git will detect copies, as well.
+ If set to "copies" or "copy", Git will detect copies, as well. If
+ set to "copies-harder", Git will try harder to detect copies.
Defaults to the value of diff.renames.
status.showStash::
diff --git a/diff.c b/diff.c
index a2def45644..585acf9a99 100644
--- a/diff.c
+++ b/diff.c
@@ -206,8 +206,11 @@ int git_config_rename(const char *var, const char *value)
{
if (!value)
return DIFF_DETECT_RENAME;
+ if (!strcasecmp(value, "copies-harder"))
+ return DIFF_DETECT_COPY_HARDER;
if (!strcasecmp(value, "copies") || !strcasecmp(value, "copy"))
- return DIFF_DETECT_COPY;
+ return DIFF_DETECT_COPY;
+
return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
}
@@ -4849,8 +4852,11 @@ void diff_setup_done(struct diff_options *options)
else
options->flags.diff_from_contents = 0;
- if (options->flags.find_copies_harder)
+ /* Just fold this in as it makes the patch-to-git smaller */
+ if (options->flags.find_copies_harder || options->detect_rename == DIFF_DETECT_COPY_HARDER) {
+ options->flags.find_copies_harder = 1;
options->detect_rename = DIFF_DETECT_COPY;
+ }
if (!options->flags.relative_name)
options->prefix = NULL;
@@ -5281,7 +5287,7 @@ static int diff_opt_find_copies(const struct option *opt,
if (*arg != 0)
return error(_("invalid argument to %s"), opt->long_name);
- if (options->detect_rename == DIFF_DETECT_COPY)
+ if (options->detect_rename == DIFF_DETECT_COPY || options->detect_rename == DIFF_DETECT_COPY_HARDER)
options->flags.find_copies_harder = 1;
else
options->detect_rename = DIFF_DETECT_COPY;
diff --git a/diff.h b/diff.h
index 66bd8aeb29..b29e5b777f 100644
--- a/diff.h
+++ b/diff.h
@@ -555,6 +555,7 @@ int git_config_rename(const char *var, const char *value);
#define DIFF_DETECT_RENAME 1
#define DIFF_DETECT_COPY 2
+#define DIFF_DETECT_COPY_HARDER 3
#define DIFF_PICKAXE_ALL 1
#define DIFF_PICKAXE_REGEX 2
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 5a6e2bcac7..856291d66f 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -299,7 +299,7 @@ static int find_identical_files(struct hashmap *srcs,
}
/* Give higher scores to sources that haven't been used already */
score = !source->rename_used;
- if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY)
+ if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY && options->detect_rename != DIFF_DETECT_COPY_HARDER)
continue;
score += basename_same(source, target);
if (score > best_score) {
@@ -1405,7 +1405,7 @@ void diffcore_rename_extended(struct diff_options *options,
trace2_region_enter("diff", "setup", options->repo);
info.setup = 0;
assert(!dir_rename_count || strmap_empty(dir_rename_count));
- want_copies = (detect_rename == DIFF_DETECT_COPY);
+ want_copies = (detect_rename == DIFF_DETECT_COPY || detect_rename == DIFF_DETECT_COPY_HARDER);
if (dirs_removed && (break_idx || want_copies))
BUG("dirs_removed incompatible with break/copy detection");
if (break_idx && relevant_sources)
diff --git a/merge-ort.c b/merge-ort.c
index 6491070d96..7749835465 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4782,7 +4782,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
* sanity check them anyway.
*/
assert(opt->detect_renames >= -1 &&
- opt->detect_renames <= DIFF_DETECT_COPY);
+ opt->detect_renames <= DIFF_DETECT_COPY_HARDER);
assert(opt->verbosity >= 0 && opt->verbosity <= 5);
assert(opt->buffer_output <= 2);
assert(opt->obuf.len == 0);
diff --git a/merge-recursive.c b/merge-recursive.c
index e3beb0801b..d52dd53660 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3708,7 +3708,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)
assert(opt->branch1 && opt->branch2);
assert(opt->detect_renames >= -1 &&
- opt->detect_renames <= DIFF_DETECT_COPY);
+ opt->detect_renames <= DIFF_DETECT_COPY_HARDER);
assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE &&
opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE);
assert(opt->rename_limit >= -1);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 00/12] Introduce `refStorage` extension
From: Junio C Hamano @ 2023-12-26 20:56 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Patrick Steinhardt, git
In-Reply-To: <CAOLa=ZR0mM_rVBa3LG-etpgYGCpiXinCZ83hcWLVeZVUhKH3UA@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> Hello,
>
> Patrick Steinhardt <ps@pks.im> writes:
>> Hi,
>>
>> this patch series introduces a new `refStorage` extension and related
>> tooling. While there is only a single user-visible ref backend for now,
>> this extension will eventually allow us to determine which backend shall
>> be used for a repository. Furthermore, the series introduces tooling so
>> that the ref storage format becomes more accessible.
>
> Apart from small nits, the series looks good to me, thanks!
Thanks. Hopefully we can merge it down after a quick, small, and
final reroll?
^ permalink raw reply
* Re: [PATCH v3] Teach git apply to respect core.fileMode settings
From: Junio C Hamano @ 2023-12-26 20:58 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Chandra Pratap via GitGitGadget, git, Torsten Bögershausen,
Chandra Pratap, Chandra Pratap
In-Reply-To: <xmqq4jg4j28z.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> As pointed out e.g. by t2016.3(git checkout -p), if the patch is to be
>> applied in reverse (`git apply -R`), then the `old_mode` is actually 0,
>> and we must use `new_mode` instead.
>
> Good finding.
Hmph, this is puzzling and I spoke way before I understand why/how
the fixup patch works X-<.
The caller of check_preimage(), check_patch(), should happen only
after reverse_patches() swapped old and new names and modes in
apply_patch(), so at this point, we should be able to consistently
use old_mode for checking, shouldn't we, even with "-R"?
^ permalink raw reply
* Re: [PATCH v3] Teach git apply to respect core.fileMode settings
From: Junio C Hamano @ 2023-12-26 21:35 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Chandra Pratap via GitGitGadget, git, Torsten Bögershausen,
Chandra Pratap, Chandra Pratap
In-Reply-To: <xmqqv88kfzpr.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> As pointed out e.g. by t2016.3(git checkout -p), if the patch is to be
>>> applied in reverse (`git apply -R`), then the `old_mode` is actually 0,
>>> and we must use `new_mode` instead.
>>
>> Good finding.
>
> Hmph, this is puzzling and I spoke way before I understand why/how
> the fixup patch works X-<.
>
> The caller of check_preimage(), check_patch(), should happen only
> after reverse_patches() swapped old and new names and modes in
> apply_patch(), so at this point, we should be able to consistently
> use old_mode for checking, shouldn't we, even with "-R"?
I think I figured it out. When we parse an incoming patch, unless
the patch is about changing the mode, we only fill old_mode (e.g.,
follow the callflow from gitdiff_index() -> gitdiff_oldmode() ->
parse_mode_line() to see how this is done). In check_patch(), which
is the caller of check_preimage() in question, there are lines that
propagate old_mode to new_mode (because unfilled new_mode can come
only because there was no mode change), but that happens much later
after check_preimage() was called and returned.
I also suspect that this copying from old to new should be done much
earlier, after parse_chunk() finds out the (old)mode by calling
find_header(), and by doing so, you wouldn't have been hit by the
"-R makes old_mode 0" because both sides would be correctly filled
before we call reverse_patches() gets called. But I haven't traced
all existing codepaths to see if such a change has unintended
consequences (e.g., somebody may incorrectly assuming that "old_mode
== new_mode == 100644" and "old_mode == 100644, new_mode == 0" mean
different things and acting differently).
In any case, I suspect that a better check is not about "are we
applying in reverse?", but more like the attached patch, i.e. if old
side is not filled, then we should pick up the other side.
apply.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git c/apply.c w/apply.c
index 697ab2eb1f..5896056a69 100644
--- c/apply.c
+++ w/apply.c
@@ -3779,12 +3779,18 @@ static int check_preimage(struct apply_state *state,
}
if (!state->cached && !previous) {
- if (!trust_executable_bit)
- st_mode = *ce ? (*ce)->ce_mode :
- (state->apply_in_reverse
- ? patch->new_mode : patch->old_mode);
- else
+ if (!trust_executable_bit) {
+ if (*ce && !(*ce)->ce_mode)
+ BUG("ce_mode == 0 for path '%s'", old_name);
+ if (*ce)
+ st_mode = (*ce)->ce_mode;
+ else if (!patch->old_mode)
+ st_mode = patch->new_mode;
+ else
+ st_mode = patch->old_mode;
+ } else {
st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ }
}
if (patch->is_new < 0)
^ permalink raw reply related
* Accessing Git
From: Marc Casanova @ 2023-12-26 22:26 UTC (permalink / raw)
To: git
I have a Git server installed on an Ubuntu container in a Proxmox
hypervisor. That part was easy.
I want to access that server from my Windows computers on the same local
network. How do I accomplish that?
^ permalink raw reply
* [PATCH v4 0/3] apply with core.filemode=false
From: Junio C Hamano @ 2023-12-26 23:32 UTC (permalink / raw)
To: git
In-Reply-To: <pull.1620.v3.git.1703066893657.gitgitgadget@gmail.com>
Chandra Pratap noticed that "git apply" on a filesystem without
executable bit support gives a warning when applying a patch that
expects the preimage file to have executable bit on. Dscho noticed
that the initial fix by Chandra did not work well when applying a
patch in reverse. It turns out that apply.c:reverse_patches()
invalidates the "a patch that does not change mode bits have the
mode bits in .old_mode member and not in .new_mode member" invariant
we rely on.
Here is the result of concerted effort.
Chandra Pratap (1):
apply: ignore working tree filemode when !core.filemode
Junio C Hamano (2):
apply: correctly reverse patch's pre- and post-image mode bits
apply: code simplification
apply.c | 16 +++++++++++++---
t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++
2 files changed, 40 insertions(+), 3 deletions(-)
--
2.43.0-174-g055bb6e996
^ permalink raw reply
* [PATCH v4 1/3] apply: ignore working tree filemode when !core.filemode
From: Junio C Hamano @ 2023-12-26 23:32 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Johannes Schindelin
In-Reply-To: <20231226233218.472054-1-gitster@pobox.com>
From: Chandra Pratap <chandrapratap3519@gmail.com>
When applying a patch that adds an executable file, git apply
ignores the core.fileMode setting (core.fileMode in git config
specifies whether the executable bit on files in the working tree
should be honored or not) resulting in warnings like:
warning: script.sh has type 100644, expected 100755
even when core.fileMode is set to false, which is undesired. This
is extra true for systems like Windows.
Fix this by inferring the correct file mode from either the existing
index entry, and when it is unavailable, assuming that the file mode
was OK by pretending it had the mode that the preimage wants to see,
when core.filemode is set to false. Add a test case that verifies
the change and prevents future regression.
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
apply.c | 10 ++++++++--
t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/apply.c b/apply.c
index 3d69fec836..3b090652cf 100644
--- a/apply.c
+++ b/apply.c
@@ -3778,8 +3778,14 @@ static int check_preimage(struct apply_state *state,
return error_errno("%s", old_name);
}
- if (!state->cached && !previous)
- st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ if (!state->cached && !previous) {
+ if (!trust_executable_bit)
+ st_mode = (*ce && (*ce)->ce_mode) ? (*ce)->ce_mode :
+ (state->apply_in_reverse
+ ? patch->new_mode : patch->old_mode);
+ else
+ st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ }
if (patch->is_new < 0)
patch->is_new = 0;
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index e7a7295f1b..e7026507dc 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -101,4 +101,31 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
)
'
+test_expect_success 'git apply respects core.fileMode' '
+ test_config core.fileMode false &&
+ echo true >script.sh &&
+ git add --chmod=+x script.sh &&
+ git ls-files -s script.sh >ls-files-output &&
+ test_grep "^100755" ls-files-output &&
+ test_tick && git commit -m "Add script" &&
+ git ls-tree -r HEAD script.sh >ls-tree-output &&
+ test_grep "^100755" ls-tree-output &&
+
+ echo true >>script.sh &&
+ test_tick && git commit -m "Modify script" script.sh &&
+ git format-patch -1 --stdout >patch &&
+ test_grep "^index.*100755$" patch &&
+
+ git switch -c branch HEAD^ &&
+ git apply --index patch 2>err &&
+ test_grep ! "has type 100644, expected 100755" err &&
+ git reset --hard &&
+
+ git apply patch 2>err &&
+ test_grep ! "has type 100644, expected 100755" err &&
+
+ git apply --cached patch 2>err &&
+ test_grep ! "has type 100644, expected 100755" err
+'
+
test_done
--
2.43.0-174-g055bb6e996
^ permalink raw reply related
* [PATCH v4 2/3] apply: correctly reverse patch's pre- and post-image mode bits
From: Junio C Hamano @ 2023-12-26 23:32 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
In-Reply-To: <20231226233218.472054-1-gitster@pobox.com>
When parsing the patch header, unless it is a patch that changes
file modes, we only read the mode bits into the .old_mode member of
the patch structure and leave .new_mode member as initialized, i.e.,
to 0. Later when we need the original mode bits, we consult .old_mode.
However, reverse_patches() that is used to swap the names and modes
of the preimage and postimage files is not aware of this convention,
leading the .old_mode to be 0 while the mode we read from the patch
is left in .new_mode.
Only swap .old_mode and .new_mode when .new_mode is not 0 (i.e. we
saw a patch that modifies the filemode and know what the new mode
is). When .new_mode is set to 0, it means the preimage and the
postimage files have the same mode (which is in the .old_mode member)
and when applying such a patch in reverse, the value in .old_mode is
what we expect the (reverse-) preimage file to have.
Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
apply.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/apply.c b/apply.c
index 3b090652cf..6b1adccb2f 100644
--- a/apply.c
+++ b/apply.c
@@ -2220,7 +2220,8 @@ static void reverse_patches(struct patch *p)
struct fragment *frag = p->fragments;
SWAP(p->new_name, p->old_name);
- SWAP(p->new_mode, p->old_mode);
+ if (p->new_mode)
+ SWAP(p->new_mode, p->old_mode);
SWAP(p->is_new, p->is_delete);
SWAP(p->lines_added, p->lines_deleted);
SWAP(p->old_oid_prefix, p->new_oid_prefix);
@@ -3780,9 +3781,8 @@ static int check_preimage(struct apply_state *state,
if (!state->cached && !previous) {
if (!trust_executable_bit)
- st_mode = (*ce && (*ce)->ce_mode) ? (*ce)->ce_mode :
- (state->apply_in_reverse
- ? patch->new_mode : patch->old_mode);
+ st_mode = (*ce && (*ce)->ce_mode)
+ ? (*ce)->ce_mode : patch->old_mode;
else
st_mode = ce_mode_from_stat(*ce, st->st_mode);
}
--
2.43.0-174-g055bb6e996
^ permalink raw reply related
* [PATCH v4 3/3] apply: code simplification
From: Junio C Hamano @ 2023-12-26 23:32 UTC (permalink / raw)
To: git
In-Reply-To: <20231226233218.472054-1-gitster@pobox.com>
Rewrite a bit hard-to-read ternary ?: expression into a cascade of
if/else.
Given that read-cache.c:add_index_entry() makes sure that the
.ce_mode member is filled with a reasonable value before placing a
cache entry in the index, if we see (ce_mode == 0), there is
something seriously wrong going on. Catch such a bug and abort,
instead of silently ignoring such an entry and silently skipping
the check.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
apply.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/apply.c b/apply.c
index 6b1adccb2f..493a263a48 100644
--- a/apply.c
+++ b/apply.c
@@ -3780,11 +3780,15 @@ static int check_preimage(struct apply_state *state,
}
if (!state->cached && !previous) {
- if (!trust_executable_bit)
- st_mode = (*ce && (*ce)->ce_mode)
- ? (*ce)->ce_mode : patch->old_mode;
- else
+ if (*ce && !(*ce)->ce_mode)
+ BUG("ce_mode == 0 for path '%s'", old_name);
+
+ if (trust_executable_bit)
st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ else if (*ce)
+ st_mode = (*ce)->ce_mode;
+ else
+ st_mode = patch->old_mode;
}
if (patch->is_new < 0)
--
2.43.0-174-g055bb6e996
^ permalink raw reply related
* subscribe to git mailing list
From: Kirandeep Paul @ 2023-12-27 1:17 UTC (permalink / raw)
To: git
subscribe git
^ permalink raw reply
* Re: [PATCH v2] diff: implement config.diff.renames=copies-harder
From: Elijah Newren @ 2023-12-27 2:24 UTC (permalink / raw)
To: Sam James; +Cc: git, Junio C Hamano
In-Reply-To: <20231226202102.3392518-1-sam@gentoo.org>
Hi,
It helps when providing a new iteration of a patch to do two things:
(1) provide a range-diff against the previous version to make it
easier for reviewers to see what has changed
(2) either reply to the previous submission or link to it in your
header so reviewers can find previous comments
In this case, v1 was over at
https://lore.kernel.org/git/pull.1606.git.1699010701704.gitgitgadget@gmail.com/.
On Tue, Dec 26, 2023 at 12:21 PM Sam James <sam@gentoo.org> wrote:
>
> This patch adds a config value for 'diff.renames' called 'copies-harder'
> which make it so '-C -C' is in effect always passed for 'git log -p',
> 'git diff', etc.
>
> This allows specifying that 'git log -p', 'git diff', etc should always act
> as if '-C --find-copies-harder' was passed.
>
> It has proven this especially useful for certain types of repository (like
> Gentoo's ebuild repositories) because files are often copies of a previous
> version:
>
> Suppose a directory 'sys-devel/gcc' contains recipes for building
> GCC, with one file for each supported upstream branch:
> gcc-13.x.build.recipe
> gcc-12.x.build.recipe
> gcc-11.x.build.recipe
> gcc-10.x.build.recipe
>
> gcc-13.x.build.recipe was started as a copy of gcc-12.x.build.recipe
> (which was started as a copy of gcc-11.x.build.recipe, etc.). Previous versions
> are kept around to support parallel installation of multiple versions.
>
> Being able to easily observe the diff relative to other recipes within the
> directory has been a quality of life improvement for such repo layouts.
>
> Cc: Elijah Newren <newren@gmail.com>
> Cc: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Sam James <sam@gentoo.org>
While the "Cc:" lines don't hurt, note that everything above this
point is included in the commit message, so we'd typically rather
those two lines be below the triple-dashed line.
> ---
> v2: Improve the commit message using Elijah Newren's example (it is indeed
> precisely what I was thinking of, but phrased better), and improve the documentation
> to explain better what the new config option actually does & refer to git-diff's
> --find-copies-harder.
>
> Documentation/config/diff.txt | 8 +++++---
> Documentation/config/status.txt | 3 ++-
> diff.c | 12 +++++++++---
> diff.h | 1 +
> diffcore-rename.c | 4 ++--
> merge-ort.c | 2 +-
> merge-recursive.c | 2 +-
> 7 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
> index bd5ae0c337..cdd8a74ec0 100644
> --- a/Documentation/config/diff.txt
> +++ b/Documentation/config/diff.txt
> @@ -131,9 +131,11 @@ diff.renames::
> Whether and how Git detects renames. If set to "false",
> rename detection is disabled. If set to "true", basic rename
> detection is enabled. If set to "copies" or "copy", Git will
> - detect copies, as well. Defaults to true. Note that this
> - affects only 'git diff' Porcelain like linkgit:git-diff[1] and
> - linkgit:git-log[1], and not lower level commands such as
> + detect copies, as well. If set to "copies-harder", Git will spend extra
> + cycles to find more copies even in unmodified paths, see
> + '--find-copies-harder' in linkgit:git-diff[1]. Defaults to true.
> + Note that this affects only 'git diff' Porcelain like linkgit:git-diff[1]
> + and linkgit:git-log[1], and not lower level commands such as
> linkgit:git-diff-files[1].
>
> diff.suppressBlankEmpty::
> diff --git a/Documentation/config/status.txt b/Documentation/config/status.txt
> index 2ff8237f8f..7ca7a4becd 100644
> --- a/Documentation/config/status.txt
> +++ b/Documentation/config/status.txt
> @@ -33,7 +33,8 @@ status.renames::
> Whether and how Git detects renames in linkgit:git-status[1] and
> linkgit:git-commit[1] . If set to "false", rename detection is
> disabled. If set to "true", basic rename detection is enabled.
> - If set to "copies" or "copy", Git will detect copies, as well.
> + If set to "copies" or "copy", Git will detect copies, as well. If
> + set to "copies-harder", Git will try harder to detect copies.
It'd be nice to have the improved text from diff.renames here ("If set
to "copies-harder", Git will spend extra cycles to find more copies
even in unmodified paths.").
> Defaults to the value of diff.renames.
>
> status.showStash::
> diff --git a/diff.c b/diff.c
> index a2def45644..585acf9a99 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -206,8 +206,11 @@ int git_config_rename(const char *var, const char *value)
> {
> if (!value)
> return DIFF_DETECT_RENAME;
> + if (!strcasecmp(value, "copies-harder"))
> + return DIFF_DETECT_COPY_HARDER;
> if (!strcasecmp(value, "copies") || !strcasecmp(value, "copy"))
> - return DIFF_DETECT_COPY;
> + return DIFF_DETECT_COPY;
> +
I pointed out in response to v1 that these last couple lines, while a
nice cleanup, should be in a separate patch.
> return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
> }
>
> @@ -4849,8 +4852,11 @@ void diff_setup_done(struct diff_options *options)
> else
> options->flags.diff_from_contents = 0;
>
> - if (options->flags.find_copies_harder)
> + /* Just fold this in as it makes the patch-to-git smaller */
> + if (options->flags.find_copies_harder || options->detect_rename == DIFF_DETECT_COPY_HARDER) {
Again, I already responded to v1 that four of your lines were too long
and needed to be split. All four remain unsplit in your resubmission.
For future reference, when you resubmit a patch, please either (a)
first respond to the review explaining why you don't agree with the
suggested changes, or (b) include the fixes reviewers point out, or
(c) state in your cover letter or explanation after the '---' why you
chose to not include the suggested changes.
> + options->flags.find_copies_harder = 1;
> options->detect_rename = DIFF_DETECT_COPY;
> + }
>
> if (!options->flags.relative_name)
> options->prefix = NULL;
> @@ -5281,7 +5287,7 @@ static int diff_opt_find_copies(const struct option *opt,
> if (*arg != 0)
> return error(_("invalid argument to %s"), opt->long_name);
>
> - if (options->detect_rename == DIFF_DETECT_COPY)
> + if (options->detect_rename == DIFF_DETECT_COPY || options->detect_rename == DIFF_DETECT_COPY_HARDER)
> options->flags.find_copies_harder = 1;
> else
> options->detect_rename = DIFF_DETECT_COPY;
> diff --git a/diff.h b/diff.h
> index 66bd8aeb29..b29e5b777f 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -555,6 +555,7 @@ int git_config_rename(const char *var, const char *value);
>
> #define DIFF_DETECT_RENAME 1
> #define DIFF_DETECT_COPY 2
> +#define DIFF_DETECT_COPY_HARDER 3
>
> #define DIFF_PICKAXE_ALL 1
> #define DIFF_PICKAXE_REGEX 2
> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index 5a6e2bcac7..856291d66f 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -299,7 +299,7 @@ static int find_identical_files(struct hashmap *srcs,
> }
> /* Give higher scores to sources that haven't been used already */
> score = !source->rename_used;
> - if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY)
> + if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY && options->detect_rename != DIFF_DETECT_COPY_HARDER)
> continue;
> score += basename_same(source, target);
> if (score > best_score) {
> @@ -1405,7 +1405,7 @@ void diffcore_rename_extended(struct diff_options *options,
> trace2_region_enter("diff", "setup", options->repo);
> info.setup = 0;
> assert(!dir_rename_count || strmap_empty(dir_rename_count));
> - want_copies = (detect_rename == DIFF_DETECT_COPY);
> + want_copies = (detect_rename == DIFF_DETECT_COPY || detect_rename == DIFF_DETECT_COPY_HARDER);
> if (dirs_removed && (break_idx || want_copies))
> BUG("dirs_removed incompatible with break/copy detection");
> if (break_idx && relevant_sources)
> diff --git a/merge-ort.c b/merge-ort.c
> index 6491070d96..7749835465 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4782,7 +4782,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
> * sanity check them anyway.
> */
> assert(opt->detect_renames >= -1 &&
> - opt->detect_renames <= DIFF_DETECT_COPY);
> + opt->detect_renames <= DIFF_DETECT_COPY_HARDER);
> assert(opt->verbosity >= 0 && opt->verbosity <= 5);
> assert(opt->buffer_output <= 2);
> assert(opt->obuf.len == 0);
> diff --git a/merge-recursive.c b/merge-recursive.c
> index e3beb0801b..d52dd53660 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -3708,7 +3708,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)
> assert(opt->branch1 && opt->branch2);
>
> assert(opt->detect_renames >= -1 &&
> - opt->detect_renames <= DIFF_DETECT_COPY);
> + opt->detect_renames <= DIFF_DETECT_COPY_HARDER);
> assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE &&
> opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE);
> assert(opt->rename_limit >= -1);
> --
> 2.43.0
The patch looks close, but it does continue to violate style
guidelines and make unrelated changes, like v1. And the wording in
one of the documentation blurbs could be improved a little. Looking
forward to a v3 with those fixed.
^ 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