* Re: [PATCH 3/3] t-ctype: do one test per class and char
From: René Scharfe @ 2024-02-26 17:26 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Phillip Wood, Josh Steadmon, Achu Luma
In-Reply-To: <CAP8UFD0Wi3ot-t0Q7ruMauwj4zkMfd89Xr9SmxYa4eQ3=2VKOw@mail.gmail.com>
Am 26.02.24 um 10:28 schrieb Christian Couder:
> On Sun, Feb 25, 2024 at 12:27 PM René Scharfe <l.s.r@web.de> wrote:
>>
>> Simplify TEST_CHAR_CLASS by using TEST for each character separately.
>> This increases the number of tests to 3598,
>
> Does this mean that when all the tests pass there will be 3598 lines
> of output on the terminal instead of 14 before this patch?
Yes.
> If that's the case, I don't like this.
>
>> but avoids the need for
>> using internal functions and test_msg() for custom messages. The
>> resulting macro has minimal test setup overhead.
>
> Yeah, the code looks definitely cleaner, but a clean output is important too.
The output is clean as well, but there's a lot of it. Perhaps too much.
The success messages are boring, though, and if all checks pass then the
only useful information is the status code. A TAP harness like prove
summarizes that nicely:
$ prove t/unit-tests/bin/t-ctype
t/unit-tests/bin/t-ctype .. ok
All tests successful.
Files=1, Tests=3598, 0 wallclock secs ( 0.08 usr + 0.00 sys = 0.08 CPU)
Result: PASS
Filtering out passing checks e.g. with "| grep -v ^ok" would help when
debugging a test failure. I vaguely miss the --immediate switch from the
regular test library, however.
René
^ permalink raw reply
* Re: [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog
From: Junio C Hamano @ 2024-02-26 17:25 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, Yasushi SHOJI, Denton Liu, Git Mailing List
In-Reply-To: <20240226101027.GA2685773@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> That's one of the reasons I split this out from patch 2; we can see
> exactly what must be done to make each case work. And in fact I had
> originally started to write a patch that simply changed t1508 to expect
> failure. I could still be persuaded to go that way if anybody feels
> strongly.
I do not feel strongly either way myself. It just is interesting
that the older end of the history is with @{20.years.ago} special
case that is only for time-based query, while the newer end of the
history is with @{0} special case.
^ permalink raw reply
* Re: [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog
From: Junio C Hamano @ 2024-02-26 17:25 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, Yasushi SHOJI, Denton Liu, Git Mailing List
In-Reply-To: <20240226100803.GC2685600@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> if (!cb.reccnt) {
> + if (cnt == 0) {
Style "if (!cnt)" ? In this particular case I do not think it
actually is an improvement, though, simply because zero is really
special in this logic.
> + /*
> + * The caller asked for ref@{0}, and we had no entries.
> + * It's a bit subtle, but in practice all callers have
> + * prepped the "oid" field with the current value of
> + * the ref, which is the most reasonable fallback.
> + *
> + * We'll put dummy values into the out-parameters (so
> + * they're not just uninitialized garbage), and the
> + * caller can take our return value as a hint that
> + * we did not find any such reflog.
> + */
> + set_read_ref_cutoffs(&cb, 0, 0, "empty reflog");
> + return 1;
> + }
The dummy value I 100% agree with ;-).
You mentioned the convenience special case for time-based reflog
query for a time older than (e.g. @{20.years.ago}) the reflog
itself, and perhaps this one should be treated as its counterpart,
that is only useful for count-based access.
^ permalink raw reply
* Re: [PATCH v5 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge
From: Jean-Noël Avila @ 2024-02-26 17:22 UTC (permalink / raw)
To: Philippe Blain, git
Cc: Johannes Sixt, Elijah Newren, Michael Lohmann, Phillip Wood,
Patrick Steinhardt, Junio C Hamano, Michael Lohmann
In-Reply-To: <20240225-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v5-1-af1ef2d9e44d@gmail.com>
Hello,
Le 25/02/2024 à 22:56, Philippe Blain a écrit :
> From: Michael Lohmann <mi.al.lohmann@gmail.com>
>
> This is done to
> (1) ensure MERGE_HEAD is a ref,
> (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
> (3) error out when MERGE_HEAD is a symref.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> revision.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/revision.c b/revision.c
> index 2424c9bd67..ee26988cc6 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1973,8 +1973,12 @@ static void prepare_show_merge(struct rev_info *revs)
> if (repo_get_oid(the_repository, "HEAD", &oid))
> die("--merge without HEAD?");
> head = lookup_commit_or_die(&oid, "HEAD");
> - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> + if (read_ref_full("MERGE_HEAD",
> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> + &oid, NULL))
> die("--merge without MERGE_HEAD?");
> + if (is_null_oid(&oid))
> + die(_("MERGE_HEAD is a symbolic ref?"));
Following the thread about being less passive-aggressive, maybe this
could be rephrased in an assertive mood.
By the way, this string is translatable, but not the one 2 lines above.
What is the policy around translation?
> other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> add_pending_object(revs, &head->object, "HEAD");
> add_pending_object(revs, &other->object, "MERGE_HEAD");
>
Thanks.
^ permalink raw reply
* Re: [Outreachy][PATCH 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string
From: Christian Couder @ 2024-02-26 17:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Achu Luma, git, Christian Couder
In-Reply-To: <xmqqil2bdvsy.fsf@gitster.g>
On Mon, Feb 26, 2024 at 5:39 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Achu Luma <ach.lumap@gmail.com> writes:
>
> > In a following commit we are going to port code from
> > "t/helper/test-sha256.c", t/helper/test-hash.c and "t/t0015-hash.sh" to
> > a new "t/unit-tests/t-hash.c" file using the recently added unit test
> > framework.
> >
> > To port code like: perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;"
> > we are going to need a new strbuf_addstrings() function that repeatedly
> > adds the same string a number of times to a buffer.
>
> We do not need to call such a function "addstrings", though. The
> name on the subject line made me expect a varargs function:
>
> (bad) strbuf_addstrings(&sb, "foo", "bar", "baz", NULL);
>
> It would have been clearer if the name hinted what it does, clearer
> than just a single "s" that says it is talking about plural. What
> would be a good name that hints "n times add a single same string"?
> I dunno.
>
> I also would have expected that the order of parameters are
> repeat-count followed by what gets repeated.
>
> Having said all of the above, we already have "addchars" that is
> equally strange, so let's let it pass ;-).
Yeah, we thought about naming it strbuf_repeatstr() first, but then
seeing addchars() I thought it would be better to imitate it.
> > diff --git a/strbuf.c b/strbuf.c
> > index 7827178d8e..eb2b3299ce 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -302,6 +302,17 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
> > strbuf_setlen(sb, sb->len + len);
> > }
> >
> > +void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n)
> > +{
> > + size_t len = strlen(s);
>
> Let's have a blank line here to separate decls from the first
> statement.
Yeah, right.
> > + if (unsigned_mult_overflows(len, n))
> > + die("you want to use way too much memory");
> > + strbuf_grow(sb, len * n);
>
> The error message given by
>
> strbuf_grow(sb, st_mult(len, n));
>
> would be equally informative and takes only a single line.
It seems that the pattern in strbuf.c, for example in strbuf_splice()
and strbuf_vinsertf(), is to do a check first using an *_overflows()
function, die if the check fails, and only then call strbuf_grow(). So
we did the same. I am fine with using your suggestion though.
> > + for (size_t i = 0; i < n; i++)
> > + memcpy(sb->buf + sb->len + len * i, s, len);
>
> Wouldn't it be sufficient to run strbuf_add() n times at this point,
> as we have already called strbuf_grow() to avoid repeated
> reallocation?
Unfortunately strbuf_add() calls strbuf_grow() itself which is not
needed as we have already called strbuf_grow() to avoid repeated
reallocation.
> Repeated manual memcpy() that involves manual offset
> computation makes me nervous.
I would have prefered to avoid it too, but didn't find a good way to do it.
> > + strbuf_setlen(sb, sb->len + len * n);
> > +}
^ permalink raw reply
* Re: [PATCH v2 03/16] t7527: temporarily disable case-insensitive tests
From: Jeff Hostetler @ 2024-02-26 17:12 UTC (permalink / raw)
To: Junio C Hamano, Jeff Hostetler via GitGitGadget
Cc: git, Patrick Steinhardt, Jeff Hostetler
In-Reply-To: <xmqqh6hz1tmx.fsf@gitster.g>
On 2/23/24 3:17 AM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Jeff Hostetler <jeffhostetler@github.com>
>>
>> Add non-existent "SKIPME" prereq to the case-insensitive tests.
>>
>> The previous commit added test cases to demonstrate an error where
>> FSMonitor can get confused on a case-insensitive file system when the
>> on-disk spelling of a file or directory is wrong. Let's disable those
>> tests before we incrementally teach Git to properly recognize and
>> handle those types of problems (so that a bisect between here and the
>> final commit in this patch series won't throw a false alarm).
>
> You talk about bisection, but hasn't the previous step already
> broken bisection without these SKIPME prerequisites? IOW, shouldn't
> this step squashed into the previous?
>
> Also, it is much more common to replace "test_expect_success" with
> "test_expect_failure" to indicate that the steps are broken. Was
> there a reason why we choose to do it differently?
In step 2 I created test with individual step failures baked into
the "! grep -q" steps in the bottom of each test. I didn't want a
failure in the 50-60 lines of setup code to cause a false alarm.
So the step 2 test "succeeds" by detecting that the output is
incomplete/wrong.
I wanted to use a "test_must_fail" on those individual grep lines
rather than a negated grep, but something complained about that
function only worked on "git" commands.
I added the SKIPME here in step 3 so that I could fix the series
in small steps and without worrying about which of the small steps
caused the file or directory case to stop being broken (which might
cause confusion if someone were bisecting in this part of the history.
Let me try again with the normal "test_expect_failure" in step 2,
drop step 3, and smash step 16 into step 14. With the rearranging
that I did in V2, both directories and files should be fixed in the
same final step -- rather than in separate steps.
Thanks
Jeff
>
>> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
>> ---
>> t/t7527-builtin-fsmonitor.sh | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
>> index 3d21295f789..4acb547819c 100755
>> --- a/t/t7527-builtin-fsmonitor.sh
>> +++ b/t/t7527-builtin-fsmonitor.sh
>> @@ -1051,7 +1051,7 @@ test_expect_success 'split-index and FSMonitor work well together' '
>> #
>> # The setup is a little contrived.
>> #
>> -test_expect_success CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
>> +test_expect_success SKIPME,CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
>> test_when_finished "stop_daemon_delete_repo subdir_case_wrong" &&
>>
>> git init subdir_case_wrong &&
>> @@ -1128,7 +1128,7 @@ test_expect_success CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
>> ! grep -q " M dir1/dir2/dir3/file3" "$PWD/subdir_case_wrong.out"
>> '
>>
>> -test_expect_success CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
>> +test_expect_success SKIPME,CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
>> test_when_finished "stop_daemon_delete_repo file_case_wrong" &&
>>
>> git init file_case_wrong &&
^ permalink raw reply
* Re: [PATCH] fetch: convert strncmp() with strlen() to starts_with()
From: Junio C Hamano @ 2024-02-26 17:11 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Patrick Steinhardt
In-Reply-To: <xmqqfrxhjujr.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> - !strncmp(rs->items[i].src,
>> - ref_namespace[NAMESPACE_TAGS].ref,
>> - strlen(ref_namespace[NAMESPACE_TAGS].ref)))) {
>> + starts_with(rs->items[i].src,
>> + ref_namespace[NAMESPACE_TAGS].ref))) {
>
> The original tries to check that "namespace" fully matches the
> initial part of .src string, which is exactly what starts_with()
> does. Makes sense.
There are two more such instances in the codebase, easily found with
$ cat >contrib/coccinelle/starts_with.cocci <<\-EOF
@@
expression string, prefix;
@@
- !strncmp(string, prefix, strlen(prefix))
+ starts_with(string, prefix)
@@
expression string, prefix;
@@
- strncmp(string, prefix, strlen(prefix))
+ !starts_with(string, prefix)
EOF
$ make contrib/coccinelle/starts_with.cocci.patch
which finds the one you just fixed (naturally, since the Cocci patch
was written by taking inspiration from your fix).
Here is one in the reftable code. The other one is in xdiff/ I'd
rather not touch (as starts_with() is probably not available there).
diff -u -p a/reftable/refname.c b/reftable/refname.c
--- a/reftable/refname.c
+++ b/reftable/refname.c
@@ -103,7 +103,7 @@ static int modification_has_ref_with_pre
}
}
- if (strncmp(ref.refname, prefix, strlen(prefix))) {
+ if (!starts_with(ref.refname, prefix)) {
err = 1;
goto done;
}
^ permalink raw reply
* Re: [PATCH] Remove a bogus empty file
From: Junio C Hamano @ 2024-02-26 16:39 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <pull.1674.git.1708958183225.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> This file has been added as part of 2232a88ab6b (attr: add builtin
> objectmode values support, 2023-11-16) and most likely serves no
> relevant purpose.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Remove a bogus empty file
>
> I happened to stumble over this today. It is a regression in v2.44.0,
> even if admittedly not a terrible one 😉
Indeed. Thanks for spotting. Will queue.
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1674%2Fdscho%2Fremove-bogus-file-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1674/dscho/remove-bogus-file-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1674
>
> neue | 0
> 1 file changed, 0 insertions(+), 0 deletions(-)
> delete mode 100644 neue
>
> diff --git a/neue b/neue
> deleted file mode 100644
> index e69de29bb2d..00000000000
>
> base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0
^ permalink raw reply
* Re: [Outreachy][PATCH 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string
From: Junio C Hamano @ 2024-02-26 16:39 UTC (permalink / raw)
To: Achu Luma; +Cc: git, christian.couder, Christian Couder
In-Reply-To: <20240226143350.3596-1-ach.lumap@gmail.com>
Achu Luma <ach.lumap@gmail.com> writes:
> In a following commit we are going to port code from
> "t/helper/test-sha256.c", t/helper/test-hash.c and "t/t0015-hash.sh" to
> a new "t/unit-tests/t-hash.c" file using the recently added unit test
> framework.
>
> To port code like: perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;"
> we are going to need a new strbuf_addstrings() function that repeatedly
> adds the same string a number of times to a buffer.
We do not need to call such a function "addstrings", though. The
name on the subject line made me expect a varargs function:
(bad) strbuf_addstrings(&sb, "foo", "bar", "baz", NULL);
It would have been clearer if the name hinted what it does, clearer
than just a single "s" that says it is talking about plural. What
would be a good name that hints "n times add a single same string"?
I dunno.
I also would have expected that the order of parameters are
repeat-count followed by what gets repeated.
Having said all of the above, we already have "addchars" that is
equally strange, so let's let it pass ;-).
> diff --git a/strbuf.c b/strbuf.c
> index 7827178d8e..eb2b3299ce 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -302,6 +302,17 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
> strbuf_setlen(sb, sb->len + len);
> }
>
> +void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n)
> +{
> + size_t len = strlen(s);
Let's have a blank line here to separate decls from the first
statement.
> + if (unsigned_mult_overflows(len, n))
> + die("you want to use way too much memory");
> + strbuf_grow(sb, len * n);
The error message given by
strbuf_grow(sb, st_mult(len, n));
would be equally informative and takes only a single line.
> + for (size_t i = 0; i < n; i++)
> + memcpy(sb->buf + sb->len + len * i, s, len);
Wouldn't it be sufficient to run strbuf_add() n times at this point,
as we have already called strbuf_grow() to avoid repeated
reallocation? Repeated manual memcpy() that involves manual offset
computation makes me nervous.
> + strbuf_setlen(sb, sb->len + len * n);
> +}
> +
> void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
> {
> strbuf_grow(sb, sb2->len);
> diff --git a/strbuf.h b/strbuf.h
> index e959caca87..0fb1b5e81e 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -310,6 +310,11 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s)
> strbuf_add(sb, s, strlen(s));
> }
>
> +/**
> + * Add a NUL-terminated string the specified number of times to the buffer.
> + */
> +void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n);
> +
> /**
> * Copy the contents of another buffer at the end of the current one.
> */
> --
> 2.43.0.windows.1
^ permalink raw reply
* Re: [PATCH] userdiff: skip textconv caching when not in a repository
From: Junio C Hamano @ 2024-02-26 16:35 UTC (permalink / raw)
To: Jeff King; +Cc: Paweł Dominiak, git
In-Reply-To: <20240226102729.GB2685773@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Subject: userdiff: skip textconv caching when not in a repository
>
> The textconv caching system uses git-notes to store its cache entries.
> But if you're using "diff --no-index" outside of a repository, then
> obviously that isn't going to work.
>
> Since caching is just an optimization, it's OK for us to skip it.
> However, the current behavior is much worse: we call notes_cache_init()
> which tries to look up the ref, and the low-level ref code hits a BUG(),
> killing the program. Instead, we should notice before setting up the
> cache that it there's no repository, and just silently skip it.
Makes sense.
Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH v2] merge-ort: turn submodule conflict suggestions into an advice
From: Junio C Hamano @ 2024-02-26 16:22 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget
Cc: git, Calvin Wan, Glen Choo, Elijah Newren,
Ævar Arnfjörð Bjarmason, Philippe Blain
In-Reply-To: <pull.1661.v2.git.git.1708954048301.gitgitgadget@gmail.com>
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> Add a new advice type 'submoduleMergeConflict' for the error message
> shown when a non-trivial submodule conflict is encountered, which was
> added in 4057523a40 (submodule merge: update conflict error message,
> 2022-08-04). That commit mentions making this message an advice as
> possible future work. Only show the advice if it is enabled.
When specifically called out like this, it makes it sound as if this
one is disabled by default, while everybody else is enabled unless it
is disabled.
> Update the tests as the expected message now appears on stderr instead
> of stdout.
Makes sense.
Thanks.
^ permalink raw reply
* Re: [Question] How to parse range-diff output
From: Junio C Hamano @ 2024-02-26 16:16 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: ZheNing Hu, Christian Couder, Johannes Schindelin, Git List
In-Reply-To: <11783b8e-472d-4b75-bb48-88138daf16ae@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> On Mon, Feb 26, 2024, at 08:25, ZheNing Hu wrote:
>> Hi,
>>
>> I am currently looking to implement a service that provides a version
>> range comparison based on git range-diff. I can easily parse out
>> commit pair headers like "3: 0bf6289 ! 3: a076e88 dev5," but I am
>> unsure how to parse the details in the subsequent diff patch body.
>>
>> It is not a standard diff output where one can parse out the filename
>> from the diff header, It should be called a diff of diffs. We can see
>> various headers with file names such as "@@ File1 (new)", "## File2
>> (new) ##", or "@@ File3: function3" in different formats. This is
>> confusing. How should we correctly parse a range-diff patch, and do
>> you have any good suggestions?
>>
>> Thanks for any help.
>> --
>> ZheNing Hu
>
> Hi
>
> Note that “Output Stability” says that this output is not meant to be
> machine-readable. It’s for human consumption. It’s not textually stable.
>
> So a new version of Git might break your implementation without warning.
Good point.
In fact, those "##" things came long after the command was
introduced for exactly the purpose of helping human users to locate
which part of a "diff" the "diff of diff" is talking about, and the
output from the command has been unstable for the exact reason.
^ permalink raw reply
* Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
From: Phillip Wood @ 2024-02-26 16:00 UTC (permalink / raw)
To: rsbecker, 'Torsten Bögershausen'; +Cc: git, Patrick Steinhardt
In-Reply-To: <5e807c1c-20a0-407b-9fc2-acd38521ba45@gmail.com>
On 26/02/2024 15:32, Phillip Wood wrote:
> Hi Randal
>
> [cc'ing Patrick for the reftable writer]
>
> On 25/02/2024 20:36, rsbecker@nexbridge.com wrote:
>> On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote:
>>> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote:
>>>> On Sunday, February 25, 2024 1:45 PM, I wrote:
>>>>> To: git@vger.kernel.org
>>> But I think that this should be used:
>>> write_in_full()
>>
>> My mailer autocorrected, yes, xwrite. write_in_full() would be safe,
>> although a bit redundant since xwrite() does similar things and is
>> used by
>> write_in_full().
>
> Note that unlike write_in_full(), xwrite() does not guarantee to write
> the whole buffer passed to it. In general unless a caller is writing a
> single byte or writing less than PIPE_BUF bytes to a pipe it should use
> write_in_full().
>
>> The question is which call is bad? The cruft stuff is
>> relatively new and I don't know the code.
I should have been clearer that I do not think any of these calls are
the likely problem for the cruft pack code. I do think the reftable
writers are worth looking at though for git in general.
For the cruft pack problem you might want to look for suspect xwrite()
calls where the caller does not handle a short write correctly for
example under builtin/ we have
builtin/index-pack.c: err = xwrite(1, input_buffer +
input_offset, input_len);
builtin/receive-pack.c: xwrite(2, msg, sz);
builtin/repack.c: xwrite(cmd->in, oid_to_hex(oid),
the_hash_algo->hexsz);
builtin/repack.c: xwrite(cmd->in, "\n", 1);
builtin/unpack-objects.c: int ret = xwrite(1, buffer +
offset, len);
Best Wishes
Phillip
>>>> reftable/writer.c: int n = w->write(w->write_arg, zeroed,
>>>> w->pending_padding);
>>>> reftable/writer.c: n = w->write(w->write_arg, data, len);
>
> Neither of these appear to check for short writes and
> reftable_fd_write() is a thin wrapper around write(). Maybe
> reftable_fd_write() should be using write_in_full()?
>
>>>> run-command.c: len = write(io->fd, io->u.out.buf,
>
> This call to write() looks correct as it is in the io pump loop.
>
>>>> t/helper/test-path-utils.c: if (write(1, buffer,
>> count)
>>>> < 0) >>> t/helper/test-windows-named-pipe.c: write(1,
>>>> buf, nbr);
>>>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
>
> In principle these all look like they are prone to short writes.
>
>>>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf,
>>>> buf_line->len);
>
> This caller explicitly says it prefers short writes over retrying
>
> Best Wishes
>
> Phillip
^ permalink raw reply
* Re: [PATCH 2/3] get_oid_basic(): special-case ref@{n} for oldest reflog entry
From: Junio C Hamano @ 2024-02-26 15:59 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, Yasushi SHOJI, Denton Liu, Git Mailing List
In-Reply-To: <20240226100407.GB2685600@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> the reflog entry we found. Instead, let's put the special case into the
> caller that resolves @{n}, as it cares only about the oid.
Thanks. I am quite puzzled why we didn't do this obvious thing from
the start.
> There is one complication, though. When read_ref_at() hits a truncated
> reflog, it will return the "old" value of the oldest entry only if it is
> not the null oid. Otherwise, it actually returns the "new" value from
> that entry! This bit of fudging is due to d1a4489a56 (avoid null SHA1 in
> oldest reflog, 2008-07-08), where asking for "ref@{20.years.ago}" for a
> ref created recently will produce the initial value as a convenience
> (even though technically it did not exist 20 years ago).
>
> But this convenience is only useful for time-based cutoffs. For
> count-based cutoffs, get_oid_basic() has always simply complained about
> going too far back:
>
> $ git rev-parse HEAD@{20}
> fatal: log for 'HEAD' only has 16 entries
>
> and we should continue to do so, rather than returning a nonsense value
> (there's even a test in t1508 already which covers this). So let's have
> the d1a4489a56 code kick in only when doing timestamp-based cutoffs.
Makes perfect sense.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> object-name.c | 9 +++++++++
> refs.c | 2 +-
> t/t1508-at-combinations.sh | 2 +-
> 3 files changed, 11 insertions(+), 2 deletions(-)
^ permalink raw reply
* RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
From: rsbecker @ 2024-02-26 15:52 UTC (permalink / raw)
To: phillip.wood, 'Torsten Bögershausen'
Cc: git, 'Patrick Steinhardt'
In-Reply-To: <5e807c1c-20a0-407b-9fc2-acd38521ba45@gmail.com>
On Monday, February 26, 2024 10:32 AM, Philip Wood wrote:
>On 25/02/2024 20:36, rsbecker@nexbridge.com wrote:
>> On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote:
>>> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote:
>>>> On Sunday, February 25, 2024 1:45 PM, I wrote:
>>>>> To: git@vger.kernel.org
>>> But I think that this should be used:
>>> write_in_full()
>>
>> My mailer autocorrected, yes, xwrite. write_in_full() would be safe,
>> although a bit redundant since xwrite() does similar things and is
>> used by write_in_full().
>
>Note that unlike write_in_full(), xwrite() does not guarantee to write the whole
>buffer passed to it. In general unless a caller is writing a single byte or writing less
>than PIPE_BUF bytes to a pipe it should use write_in_full().
>
>> The question is which call is bad? The cruft stuff is relatively new
>> and I don't know the code.
>>
>>>> reftable/writer.c: int n = w->write(w->write_arg, zeroed,
>>>> w->pending_padding);
>>>> reftable/writer.c: n = w->write(w->write_arg, data, len);
>
>Neither of these appear to check for short writes and
>reftable_fd_write() is a thin wrapper around write(). Maybe
>reftable_fd_write() should be using write_in_full()?
>
>>>> run-command.c: len = write(io->fd, io->u.out.buf,
>
>This call to write() looks correct as it is in the io pump loop.
>
>>>> t/helper/test-path-utils.c: if (write(1, buffer,
>> count)
>>>> < 0) >>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
>>>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
>
>In principle these all look like they are prone to short writes.
>
>>>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, buf_line->len);
>
>This caller explicitly says it prefers short writes over retrying
The real issue is that t7704.9 fails as follows:
Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0) Enumerating objects: 3, done.
Counting objects: 100% (3/3), done.
Writing objects: 100% (3/3), done.
Total 3 (delta 0), reused 3 (delta 0), pack-reused 0 (from 0)
ls: cannot access '.git/objects/pack/pack-*.mtimes': No such file or directory
test_line_count: line count for cruft.after != 2 not ok 9 - --max-cruft-size with pruning #
So something is not writing the mtimes file correctly. That's what I am trying to track down. The write issue is a possible cause but not necessarily the root cause.
--Randall
^ permalink raw reply
* Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
From: Phillip Wood @ 2024-02-26 15:32 UTC (permalink / raw)
To: rsbecker, 'Torsten Bögershausen'; +Cc: git, Patrick Steinhardt
In-Reply-To: <01ca01da682a$5f6a7b60$1e3f7220$@nexbridge.com>
Hi Randal
[cc'ing Patrick for the reftable writer]
On 25/02/2024 20:36, rsbecker@nexbridge.com wrote:
> On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote:
>> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote:
>>> On Sunday, February 25, 2024 1:45 PM, I wrote:
>>>> To: git@vger.kernel.org
>> But I think that this should be used:
>> write_in_full()
>
> My mailer autocorrected, yes, xwrite. write_in_full() would be safe,
> although a bit redundant since xwrite() does similar things and is used by
> write_in_full().
Note that unlike write_in_full(), xwrite() does not guarantee to write
the whole buffer passed to it. In general unless a caller is writing a
single byte or writing less than PIPE_BUF bytes to a pipe it should use
write_in_full().
> The question is which call is bad? The cruft stuff is
> relatively new and I don't know the code.
>
>>> reftable/writer.c: int n = w->write(w->write_arg, zeroed,
>>> w->pending_padding);
>>> reftable/writer.c: n = w->write(w->write_arg, data, len);
Neither of these appear to check for short writes and
reftable_fd_write() is a thin wrapper around write(). Maybe
reftable_fd_write() should be using write_in_full()?
>>> run-command.c: len = write(io->fd, io->u.out.buf,
This call to write() looks correct as it is in the io pump loop.
>>> t/helper/test-path-utils.c: if (write(1, buffer,
> count)
>>> < 0) >>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
>>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
In principle these all look like they are prone to short writes.
>>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, buf_line->len);
This caller explicitly says it prefers short writes over retrying
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH] unit-tests: convert t/helper/test-oid-array.c to unit-tests
From: Christian Couder @ 2024-02-26 15:11 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <20240223193257.9222-1-shyamthakkar001@gmail.com>
On Fri, Feb 23, 2024 at 8:33 PM Ghanshyam Thakkar
<shyamthakkar001@gmail.com> wrote:
>
> Migrate t/helper/test-oid-array.c and t/t0064-oid-array.sh to the
> recently added unit testing framework. This would improve runtime
> performance and provide better debugging via displaying array content,
> index at which the test failed etc. directly to stdout.
It might not be a good idea to start working on a GSoC project we
propose (Move existing tests to a unit testing framework) right now.
You can work on it as part of your GSoC application, to show an
example of how you would do it, and we might review that as part of
reviewing your application. But for such a project if many candidates
started working on it and sent patches to the mailing list before they
get selected, then the project might be nearly finished before the
GSoC even starts.
So I think it would be better to work on other things instead, like
perhaps reviewing other people's work or working on other bug fixes or
features. Anyway now that this is on the mailing list, I might as well
review it as it could help with your application. But please consider
working on other things.
> There is only one change in the new testing approach. In the previous
> testing method, a new repo gets initialized for the test according to
> GIT_TEST_DEFAULT_HASH algorithm.
It looks like this happens in "t/test-lib-functions.sh", right?
Telling a bit more about how and where that happens might help
reviewers who would like to take a look.
> In unit testing however, we do not
> need to initialize the repo. We can set the length of the hexadecimal
> strbuf according to the algorithm used directly.
So is your patch doing that or not? It might be better to be explicit.
Also if 'strbuf's are used, then is it really worth it to set their
length in advance, instead of just letting them grow to the right
length as we add hex to them?
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
> [RFC]: I recently saw a series by Eric W. Biederman [1] which enables
> the use of oid's with different hash algorithms into the same
> oid_array safely. However, there were no tests added for this. So, I
> am wondering if we should have a input format which allows us to
> specify hash algo for each oid with its hex value. i.e. "sha1:55" or
> "sha256:55", instead of just "55" and relying on GIT_TEST_DEFAULT_HASH
> for algo. So far, I tried to imitate the existing tests but I suppose
> this may be useful in the future if that series gets merged.
The fact that there is a series touching the same area might also hint
that it might not be the right time to work on this.
> diff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c
> new file mode 100644
> index 0000000000..b4f43c025d
> --- /dev/null
> +++ b/t/unit-tests/t-oid-array.c
> @@ -0,0 +1,222 @@
> +#include "test-lib.h"
> +#include "hex.h"
> +#include "oid-array.h"
> +#include "strbuf.h"
> +
> +#define INPUT "88", "44", "aa", "55"
> +#define INPUT_DUP \
> + "88", "44", "aa", "55", "88", "44", "aa", "55", "88", "44", "aa", "55"
Can you reuse INPUT in INPUT_DUP?
> +#define INPUT_ONLY_DUP "55", "55"
> +#define ENUMERATION_RESULT_SORTED "44", "55", "88", "aa"
> +
> +/*
> + * allocates the memory based on the hash algorithm used and sets the length to
> + * it.
> + */
> +static void hex_strbuf_init(struct strbuf *hex)
> +{
> + static int sz = -1;
> +
> + if (sz == -1) {
> + char *algo_env = getenv("GIT_TEST_DEFAULT_HASH");
> + if (algo_env && !strcmp(algo_env, "sha256"))
> + sz = GIT_SHA256_HEXSZ;
> + else
> + sz = GIT_SHA1_HEXSZ;
> + }
> +
> + strbuf_init(hex, sz);
> + strbuf_setlen(hex, sz);
> +}
A strbuf can grow when we add stuff to it. We don't need to know its
size in advance. So I am not sure this function is actually useful.
> +/* fills the hex strbuf with alternating characters from 'c' */
> +static void fill_hex_strbuf(struct strbuf *hex, char *c)
> +{
> + size_t i;
> + for (i = 0; i < hex->len; i++)
> + hex->buf[i] = (i & 1) ? c[1] : c[0];
There is strbuf_addch() to add a single char to a strbuf, or
strbuf_add() and strbuf_addstr() to add many chars at once.
> +}
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)
From: Taylor Blau @ 2024-02-26 14:44 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Junio C Hamano, git
In-Reply-To: <20240225225918.GB1940392@szeder.dev>
Hi SZEDER,
On Sun, Feb 25, 2024 at 11:59:18PM +0100, SZEDER Gábor wrote:
> It's great that we finally have test cases for different Bloom filter
> settings in different commit-graph layers, including a test case that
> merges those layers, but that test case doesn't check that the
> resulting merged commit-graph file contains the right settings. And
> there is still no test case that merges layers with different Bloom
> filter versions.
Thanks for reviewing.
I'm happy to produce another round of this series that would address
what you've added here.
Are there any other specific things you'd like to see addressed in a
subsequent round? I want to make sure that I'm addressing all of your
concerns, and avoid the need for yet another round of this series that
addresses things that I could have done in the first place.
> I think adding these would be the bare minimum... and would need more
> for due diligence.
As I said above, I'm happy to add these things in, but please do let me
know if there are others.
Thanks,
Taylor
^ permalink raw reply
* [PATCH] Remove a bogus empty file
From: Johannes Schindelin via GitGitGadget @ 2024-02-26 14:36 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
This file has been added as part of 2232a88ab6b (attr: add builtin
objectmode values support, 2023-11-16) and most likely serves no
relevant purpose.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Remove a bogus empty file
I happened to stumble over this today. It is a regression in v2.44.0,
even if admittedly not a terrible one 😉
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1674%2Fdscho%2Fremove-bogus-file-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1674/dscho/remove-bogus-file-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1674
neue | 0
1 file changed, 0 insertions(+), 0 deletions(-)
delete mode 100644 neue
diff --git a/neue b/neue
deleted file mode 100644
index e69de29bb2d..00000000000
base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0
--
gitgitgadget
^ permalink raw reply
* [Outreachy][PATCH 2/2] Port helper/test-sha256.c and helper/test-sha1.c to unit-tests/t-hash.c
From: Achu Luma @ 2024-02-26 14:33 UTC (permalink / raw)
To: git; +Cc: christian.couder, Achu Luma, Christian Couder
In-Reply-To: <20240226143350.3596-1-ach.lumap@gmail.com>
In the recent codebase update (8bf6fbd (Merge branch
'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
merged, providing a standardized approach for testing C code. Prior to
this update, some unit tests relied on the test helper mechanism,
lacking a dedicated unit testing framework. It's more natural to perform
these unit tests using the new unit test framework.
This commit migrates the unit tests for hash functionality from the
legacy approach using the test-tool command `test-tool sha1`and
`test-tool sha256` in helper/test-sha256.c and helper/test-sha1.c to the
new unit testing framework (t/unit-tests/test-lib.h). Porting
t0013-sha1dc.sh is left for later.
The migration involves refactoring the tests to utilize the testing
macros provided by the framework (TEST() and check_*()).
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
Makefile | 2 +-
t/helper/test-tool.c | 1 -
t/helper/test-tool.h | 1 -
t/t0015-hash.sh | 56 --------------------------------
t/unit-tests/t-hash.c | 75 +++++++++++++++++++++++++++++++++++++++++++
5 files changed, 76 insertions(+), 59 deletions(-)
delete mode 100755 t/t0015-hash.sh
create mode 100644 t/unit-tests/t-hash.c
diff --git a/Makefile b/Makefile
index 78e874099d..b73d2270f9 100644
--- a/Makefile
+++ b/Makefile
@@ -847,7 +847,6 @@ TEST_BUILTINS_OBJS += test-run-command.o
TEST_BUILTINS_OBJS += test-scrap-cache-tree.o
TEST_BUILTINS_OBJS += test-serve-v2.o
TEST_BUILTINS_OBJS += test-sha1.o
-TEST_BUILTINS_OBJS += test-sha256.o
TEST_BUILTINS_OBJS += test-sigchain.o
TEST_BUILTINS_OBJS += test-simple-ipc.o
TEST_BUILTINS_OBJS += test-strcmp-offset.o
@@ -1346,6 +1345,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
UNIT_TEST_PROGRAMS += t-strbuf
UNIT_TEST_PROGRAMS += t-ctype
UNIT_TEST_PROGRAMS += t-prio-queue
+UNIT_TEST_PROGRAMS += t-hash
UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 482a1e58a4..7bfbb75c9b 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -73,7 +73,6 @@ static struct test_cmd cmds[] = {
{ "serve-v2", cmd__serve_v2 },
{ "sha1", cmd__sha1 },
{ "sha1-is-sha1dc", cmd__sha1_is_sha1dc },
- { "sha256", cmd__sha256 },
{ "sigchain", cmd__sigchain },
{ "simple-ipc", cmd__simple_ipc },
{ "strcmp-offset", cmd__strcmp_offset },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index b1be7cfcf5..8139c9664d 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -66,7 +66,6 @@ int cmd__serve_v2(int argc, const char **argv);
int cmd__sha1(int argc, const char **argv);
int cmd__sha1_is_sha1dc(int argc, const char **argv);
int cmd__oid_array(int argc, const char **argv);
-int cmd__sha256(int argc, const char **argv);
int cmd__sigchain(int argc, const char **argv);
int cmd__simple_ipc(int argc, const char **argv);
int cmd__strcmp_offset(int argc, const char **argv);
diff --git a/t/t0015-hash.sh b/t/t0015-hash.sh
deleted file mode 100755
index 0a087a1983..0000000000
--- a/t/t0015-hash.sh
+++ /dev/null
@@ -1,56 +0,0 @@
-#!/bin/sh
-
-test_description='test basic hash implementation'
-
-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
-
-test_expect_success 'test basic SHA-1 hash values' '
- test-tool sha1 </dev/null >actual &&
- grep da39a3ee5e6b4b0d3255bfef95601890afd80709 actual &&
- printf "a" | test-tool sha1 >actual &&
- grep 86f7e437faa5a7fce15d1ddcb9eaeaea377667b8 actual &&
- printf "abc" | test-tool sha1 >actual &&
- grep a9993e364706816aba3e25717850c26c9cd0d89d actual &&
- printf "message digest" | test-tool sha1 >actual &&
- grep c12252ceda8be8994d5fa0290a47231c1d16aae3 actual &&
- printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha1 >actual &&
- grep 32d10c7b8cf96570ca04ce37f2a19d84240d3a89 actual &&
- perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;" |
- test-tool sha1 >actual &&
- grep 34aa973cd4c4daa4f61eeb2bdbad27316534016f actual &&
- printf "blob 0\0" | test-tool sha1 >actual &&
- grep e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 actual &&
- printf "blob 3\0abc" | test-tool sha1 >actual &&
- grep f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f actual &&
- printf "tree 0\0" | test-tool sha1 >actual &&
- grep 4b825dc642cb6eb9a060e54bf8d69288fbee4904 actual
-'
-
-test_expect_success 'test basic SHA-256 hash values' '
- test-tool sha256 </dev/null >actual &&
- grep e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 actual &&
- printf "a" | test-tool sha256 >actual &&
- grep ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb actual &&
- printf "abc" | test-tool sha256 >actual &&
- grep ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad actual &&
- printf "message digest" | test-tool sha256 >actual &&
- grep f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650 actual &&
- printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha256 >actual &&
- grep 71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73 actual &&
- # Try to exercise the chunking code by turning autoflush on.
- perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;" |
- test-tool sha256 >actual &&
- grep cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0 actual &&
- perl -e "$| = 1; print q{abcdefghijklmnopqrstuvwxyz} for 1..100000;" |
- test-tool sha256 >actual &&
- grep e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35 actual &&
- printf "blob 0\0" | test-tool sha256 >actual &&
- grep 473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813 actual &&
- printf "blob 3\0abc" | test-tool sha256 >actual &&
- grep c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6 actual &&
- printf "tree 0\0" | test-tool sha256 >actual &&
- grep 6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321 actual
-'
-
-test_done
diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
new file mode 100644
index 0000000000..b512e64bf1
--- /dev/null
+++ b/t/unit-tests/t-hash.c
@@ -0,0 +1,75 @@
+#include "test-lib.h"
+#include "hash-ll.h"
+#include "hex.h"
+#include "strbuf.h"
+
+static void check_hash_data(const void *data, size_t data_length, const char *expected, int algo) {
+ git_hash_ctx ctx;
+ unsigned char hash[GIT_MAX_HEXSZ];
+ const struct git_hash_algo *algop = &hash_algos[algo];
+
+ if (!check(!!data)) {
+ test_msg("Error: No data provided when expecting: %s", expected);
+ return;
+ }
+
+ algop->init_fn(&ctx);
+ algop->update_fn(&ctx, data, data_length);
+ algop->final_fn(hash, &ctx);
+
+ check_str(hash_to_hex_algop(hash, algop), expected);
+}
+
+/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
+#define TEST_SHA1_STR(data, expected) \
+ TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA1), \
+ "SHA1 (%s) works", #data)
+
+/* Only works with a literal string, useful when it contains a NUL character. */
+#define TEST_SHA1_LITERAL(literal, expected) \
+ TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
+ "SHA1 (%s) works", #literal)
+
+/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
+#define TEST_SHA256_STR(data, expected) \
+ TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA256), \
+ "SHA256 (%s) works", #data)
+
+/* Only works with a literal string, useful when it contains a NUL character. */
+#define TEST_SHA256_LITERAL(literal, expected) \
+ TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA256), \
+ "SHA256 (%s) works", #literal)
+
+int cmd_main(int argc, const char **argv) {
+ struct strbuf aaaaaaaaaa_100000 = STRBUF_INIT;
+ struct strbuf alphabet_100000 = STRBUF_INIT;
+
+ strbuf_addstrings(&aaaaaaaaaa_100000, "aaaaaaaaaa", 100000);
+ strbuf_addstrings(&alphabet_100000, "abcdefghijklmnopqrstuvwxyz", 100000);
+
+ TEST_SHA1_STR("", "da39a3ee5e6b4b0d3255bfef95601890afd80709");
+ TEST_SHA1_STR("a", "86f7e437faa5a7fce15d1ddcb9eaeaea377667b8");
+ TEST_SHA1_STR("abc", "a9993e364706816aba3e25717850c26c9cd0d89d");
+ TEST_SHA1_STR("message digest", "c12252ceda8be8994d5fa0290a47231c1d16aae3");
+ TEST_SHA1_STR("abcdefghijklmnopqrstuvwxyz", "32d10c7b8cf96570ca04ce37f2a19d84240d3a89");
+ TEST_SHA1_STR(aaaaaaaaaa_100000.buf, "34aa973cd4c4daa4f61eeb2bdbad27316534016f");
+ TEST_SHA1_LITERAL("blob 0\0", "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391");
+ TEST_SHA1_LITERAL("blob 3\0abc", "f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f");
+ TEST_SHA1_LITERAL("tree 0\0", "4b825dc642cb6eb9a060e54bf8d69288fbee4904");
+
+ TEST_SHA256_STR("", "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
+ TEST_SHA256_STR("a", "ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb");
+ TEST_SHA256_STR("abc", "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
+ TEST_SHA256_STR("message digest", "f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650");
+ TEST_SHA256_STR("abcdefghijklmnopqrstuvwxyz", "71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73");
+ TEST_SHA256_STR(aaaaaaaaaa_100000.buf, "cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0");
+ TEST_SHA256_STR(alphabet_100000.buf, "e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35");
+ TEST_SHA256_LITERAL("blob 0\0", "473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813");
+ TEST_SHA256_LITERAL("blob 3\0abc", "c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6");
+ TEST_SHA256_LITERAL("tree 0\0", "6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321");
+
+ strbuf_release(&aaaaaaaaaa_100000);
+ strbuf_release(&alphabet_100000);
+
+ return test_done();
+}
--
2.43.0.windows.1
^ permalink raw reply related
* [Outreachy][PATCH 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string
From: Achu Luma @ 2024-02-26 14:33 UTC (permalink / raw)
To: git; +Cc: christian.couder, Achu Luma, Christian Couder
In a following commit we are going to port code from
"t/helper/test-sha256.c", t/helper/test-hash.c and "t/t0015-hash.sh" to
a new "t/unit-tests/t-hash.c" file using the recently added unit test
framework.
To port code like: perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;"
we are going to need a new strbuf_addstrings() function that repeatedly
adds the same string a number of times to a buffer.
Such a strbuf_addstrings() function would already be useful in
"json-writer.c" and "builtin/submodule-helper.c" as both of these files
already have code that repeatedly adds the same string. So let's
introduce such a strbuf_addstrings() function in "strbuf.{c,h}" and use
it in both "json-writer.c" and "builtin/submodule-helper.c".
We use the "strbuf_addstrings" name as this way strbuf_addstr() and
strbuf_addstrings() would be similar for strings as strbuf_addch() and
strbuf_addchars() for characters.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
builtin/submodule--helper.c | 4 +---
json-writer.c | 5 +----
strbuf.c | 11 +++++++++++
strbuf.h | 5 +++++
4 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fda50f2af1..bed08af410 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -257,11 +257,9 @@ static void module_list_active(struct module_list *list)
static char *get_up_path(const char *path)
{
- int i;
struct strbuf sb = STRBUF_INIT;
- for (i = count_slashes(path); i; i--)
- strbuf_addstr(&sb, "../");
+ strbuf_addstrings(&sb, "../", count_slashes(path));
/*
* Check if 'path' ends with slash or not
diff --git a/json-writer.c b/json-writer.c
index 005c820aa4..25b9201f9c 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -46,10 +46,7 @@ static void append_quoted_string(struct strbuf *out, const char *in)
static void indent_pretty(struct json_writer *jw)
{
- int k;
-
- for (k = 0; k < jw->open_stack.len; k++)
- strbuf_addstr(&jw->json, " ");
+ strbuf_addstrings(&jw->json, " ", jw->open_stack.len);
}
/*
diff --git a/strbuf.c b/strbuf.c
index 7827178d8e..eb2b3299ce 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -302,6 +302,17 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
strbuf_setlen(sb, sb->len + len);
}
+void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n)
+{
+ size_t len = strlen(s);
+ if (unsigned_mult_overflows(len, n))
+ die("you want to use way too much memory");
+ strbuf_grow(sb, len * n);
+ for (size_t i = 0; i < n; i++)
+ memcpy(sb->buf + sb->len + len * i, s, len);
+ strbuf_setlen(sb, sb->len + len * n);
+}
+
void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
{
strbuf_grow(sb, sb2->len);
diff --git a/strbuf.h b/strbuf.h
index e959caca87..0fb1b5e81e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -310,6 +310,11 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s)
strbuf_add(sb, s, strlen(s));
}
+/**
+ * Add a NUL-terminated string the specified number of times to the buffer.
+ */
+void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n);
+
/**
* Copy the contents of another buffer at the end of the current one.
*/
--
2.43.0.windows.1
^ permalink raw reply related
* [PATCH v2] merge-ort: turn submodule conflict suggestions into an advice
From: Philippe Blain via GitGitGadget @ 2024-02-26 13:27 UTC (permalink / raw)
To: git
Cc: Calvin Wan, Glen Choo, Elijah Newren,
Ævar Arnfjörð Bjarmason, Philippe Blain,
Philippe Blain
In-Reply-To: <pull.1661.git.git.1706534968589.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
Add a new advice type 'submoduleMergeConflict' for the error message
shown when a non-trivial submodule conflict is encountered, which was
added in 4057523a40 (submodule merge: update conflict error message,
2022-08-04). That commit mentions making this message an advice as
possible future work. Only show the advice if it is enabled.
Update the tests as the expected message now appears on stderr instead
of stdout.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
merge-ort: turn submodule conflict suggestions into an advice
Changes since v1:
* Rebased onto v2.44.0 to solve conflicts
* CC'ed more people involved in the thread where the message was added
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1661%2Fphil-blain%2Fmerge-submodule-conflict-advice-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1661/phil-blain/merge-submodule-conflict-advice-v2
Pull-Request: https://github.com/git/git/pull/1661
Range-diff vs v1:
1: e77f3453fe1 ! 1: a6d631e0c69 merge-ort: turn submodule conflict suggestions into an advice
@@ Commit message
shown when a non-trivial submodule conflict is encountered, which was
added in 4057523a40 (submodule merge: update conflict error message,
2022-08-04). That commit mentions making this message an advice as
- possible future work.
+ possible future work. Only show the advice if it is enabled.
Update the tests as the expected message now appears on stderr instead
of stdout.
@@ Documentation/config/advice.txt: advice.*::
## advice.c ##
@@ advice.c: static struct {
- [ADVICE_STATUS_U_OPTION] = { "statusUoption", 1 },
- [ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated", 1 },
- [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
-+ [ADVICE_SUBMODULE_MERGE_CONFLICT] = { "submoduleMergeConflict", 1 },
- [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead", 1 },
- [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 },
- [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 },
+ [ADVICE_STATUS_U_OPTION] = { "statusUoption" },
+ [ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated" },
+ [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie" },
++ [ADVICE_SUBMODULE_MERGE_CONFLICT] = { "submoduleMergeConflict" },
+ [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead" },
+ [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath" },
+ [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor" },
## advice.h ##
@@ advice.h: enum advice_type {
Documentation/config/advice.txt | 3 +++
advice.c | 1 +
advice.h | 1 +
merge-ort.c | 3 ++-
t/t6437-submodule-merge.sh | 14 +++++++-------
t/t7402-submodule-rebase.sh | 2 +-
6 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c7ea70f2e2e..dde8e7840ef 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -129,6 +129,9 @@ advice.*::
submoduleAlternateErrorStrategyDie::
Advice shown when a submodule.alternateErrorStrategy option
configured to "die" causes a fatal error.
+ submoduleMergeConflict::
+ Advice shown when a non-trivial submodule merge conflict is
+ encountered.
submodulesNotUpdated::
Advice shown when a user runs a submodule command that fails
because `git submodule update --init` was not run.
diff --git a/advice.c b/advice.c
index 6e9098ff089..4ba64ee5b2d 100644
--- a/advice.c
+++ b/advice.c
@@ -79,6 +79,7 @@ static struct {
[ADVICE_STATUS_U_OPTION] = { "statusUoption" },
[ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated" },
[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie" },
+ [ADVICE_SUBMODULE_MERGE_CONFLICT] = { "submoduleMergeConflict" },
[ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead" },
[ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath" },
[ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor" },
diff --git a/advice.h b/advice.h
index 9d4f49ae38b..7d0a821f5cb 100644
--- a/advice.h
+++ b/advice.h
@@ -47,6 +47,7 @@ enum advice_type {
ADVICE_STATUS_U_OPTION,
ADVICE_SUBMODULES_NOT_UPDATED,
ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
+ ADVICE_SUBMODULE_MERGE_CONFLICT,
ADVICE_SUGGEST_DETACHING_HEAD,
ADVICE_UPDATE_SPARSE_PATH,
ADVICE_WAITING_FOR_EDITOR,
diff --git a/merge-ort.c b/merge-ort.c
index 8617babee41..6a48aea2275 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -18,6 +18,7 @@
#include "merge-ort.h"
#include "alloc.h"
+#include "advice.h"
#include "attr.h"
#include "cache-tree.h"
#include "commit.h"
@@ -4556,7 +4557,7 @@ static void print_submodule_conflict_suggestion(struct string_list *csub) {
" - commit the resulting index in the superproject\n"),
tmp.buf, subs.buf);
- printf("%s", msg.buf);
+ advise_if_enabled(ADVICE_SUBMODULE_MERGE_CONFLICT, "%s", msg.buf);
strbuf_release(&subs);
strbuf_release(&tmp);
diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh
index 70650521b04..7a3f1cb27c1 100755
--- a/t/t6437-submodule-merge.sh
+++ b/t/t6437-submodule-merge.sh
@@ -113,7 +113,7 @@ test_expect_success 'merging should conflict for non fast-forward' '
git checkout -b test-nonforward-a b &&
if test "$GIT_TEST_MERGE_ALGORITHM" = ort
then
- test_must_fail git merge c >actual &&
+ test_must_fail git merge c 2>actual &&
sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" &&
grep "$sub_expect" actual
else
@@ -154,9 +154,9 @@ test_expect_success 'merging should conflict for non fast-forward (resolution ex
git rev-parse --short sub-d > ../expect) &&
if test "$GIT_TEST_MERGE_ALGORITHM" = ort
then
- test_must_fail git merge c >actual &&
+ test_must_fail git merge c >actual 2>sub-actual &&
sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" &&
- grep "$sub_expect" actual
+ grep "$sub_expect" sub-actual
else
test_must_fail git merge c 2> actual
fi &&
@@ -181,9 +181,9 @@ test_expect_success 'merging should fail for ambiguous common parent' '
) &&
if test "$GIT_TEST_MERGE_ALGORITHM" = ort
then
- test_must_fail git merge c >actual &&
+ test_must_fail git merge c >actual 2>sub-actual &&
sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" &&
- grep "$sub_expect" actual
+ grep "$sub_expect" sub-actual
else
test_must_fail git merge c 2> actual
fi &&
@@ -227,7 +227,7 @@ test_expect_success 'merging should fail for changes that are backwards' '
git commit -a -m "f" &&
git checkout -b test-backward e &&
- test_must_fail git merge f >actual &&
+ test_must_fail git merge f 2>actual &&
if test "$GIT_TEST_MERGE_ALGORITHM" = ort
then
sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-d)" &&
@@ -535,7 +535,7 @@ test_expect_success 'merging should fail with no merge base' '
git checkout -b b init &&
git add sub &&
git commit -m "b" &&
- test_must_fail git merge a >actual &&
+ test_must_fail git merge a 2>actual &&
if test "$GIT_TEST_MERGE_ALGORITHM" = ort
then
sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short HEAD^1)" &&
diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
index 2b3c363078b..aa2fdc31d1a 100755
--- a/t/t7402-submodule-rebase.sh
+++ b/t/t7402-submodule-rebase.sh
@@ -116,7 +116,7 @@ test_expect_success 'rebasing submodule that should conflict' '
test_tick &&
git commit -m fourth &&
- test_must_fail git rebase --onto HEAD^^ HEAD^ HEAD^0 >actual_output &&
+ test_must_fail git rebase --onto HEAD^^ HEAD^ HEAD^0 2>actual_output &&
git ls-files -s submodule >actual &&
(
cd submodule &&
base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0
--
gitgitgadget
^ permalink raw reply related
* Re: [Question] How to parse range-diff output
From: Kristoffer Haugsbakk @ 2024-02-26 13:16 UTC (permalink / raw)
To: ZheNing Hu
Cc: Junio C Hamano, Christian Couder, Johannes Schindelin, Git List
In-Reply-To: <CAOLTT8SQyBSWC=aqB2SRYmp3kR6RZ+L_-9yckWQf-X9rbzeNBw@mail.gmail.com>
On Mon, Feb 26, 2024, at 08:25, ZheNing Hu wrote:
> Hi,
>
> I am currently looking to implement a service that provides a version
> range comparison based on git range-diff. I can easily parse out
> commit pair headers like "3: 0bf6289 ! 3: a076e88 dev5," but I am
> unsure how to parse the details in the subsequent diff patch body.
>
> It is not a standard diff output where one can parse out the filename
> from the diff header, It should be called a diff of diffs. We can see
> various headers with file names such as "@@ File1 (new)", "## File2
> (new) ##", or "@@ File3: function3" in different formats. This is
> confusing. How should we correctly parse a range-diff patch, and do
> you have any good suggestions?
>
> Thanks for any help.
> --
> ZheNing Hu
Hi
Note that “Output Stability” says that this output is not meant to be
machine-readable. It’s for human consumption. It’s not textually stable.
So a new version of Git might break your implementation without warning.
--
Kristoffer Haugsbakk
^ permalink raw reply
* [PATCH] replay: Fix documentation of the --contained flag
From: Gabriel de Perthuis @ 2024-02-26 11:32 UTC (permalink / raw)
To: g2p.code, git; +Cc: Elijah Newren
There was a paragraph describing the behaviour of that flag (which
affects branches and refs), but referring to it as --onto.
--contained is used with --onto but it is a more specific flag.
--onto by itself refers to a commit and does not have the described
effect on branches.
Signed-off-by: Gabriel de Perthuis <g2p.code@gmail.com>
---
Documentation/git-replay.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-replay.txt b/Documentation/git-replay.txt
index f6c269c62d..ab28e86a4a 100644
--- a/Documentation/git-replay.txt
+++ b/Documentation/git-replay.txt
@@ -27,12 +27,12 @@ OPTIONS
--onto <newbase>::
Starting point at which to create the new commits. May be any
valid commit, and not just an existing branch name.
+
-When `--onto` is specified, the update-ref command(s) in the output will
-update the branch(es) in the revision range to point at the new
+When `--contained` is specified, the update-ref command(s) in the output will
+update any branch(es) pointing within the revision range to point at the new
commits, similar to the way how `git rebase --update-refs` updates
multiple branches in the affected range.
--advance <branch>::
Starting point at which to create the new commits; must be a
---
base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0
change-id: 20240226-fix-replay-docs-70a54ea9e3c8
Best regards,
--
Gabriel de Perthuis
^ permalink raw reply related
* [PATCH] rebase -i: improve error message when picking merge
From: Phillip Wood via GitGitGadget @ 2024-02-26 10:58 UTC (permalink / raw)
To: git; +Cc: Stefan Haller, Johannes Schindelin, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
The only todo commands that accept a merge commit are "merge" and
"reset". All the other commands like "pick" or "reword" fail when they
try to pick a a merge commit and print the message
error: commit abc123 is a merge but no -m option was given.
followed by a hint about the command being rescheduled. This message is
designed to help the user when they cherry-pick a merge and forget to
pass "-m". For users who are rebasing the message is confusing as there
is no way for rebase to cherry-pick the merge.
Improve the user experience by detecting the error when the todo list is
parsed rather than waiting for the "pick" command to fail and print a
message recommending the "merge" command instead. We recommend "merge"
rather than "exec git cherry-pick -m ..." on the assumption that
cherry-picking merges is relatively rare and it is more likely that the
user chose "pick" by a mistake.
It would be possible to support cherry-picking merges by allowing the
user to pass "-m" to "pick" commands but that adds complexity to do
something that can already be achieved with
exec git cherry-pick -m1 abc123
The change is relatively straight forward but is complicated slightly as
we now need to tell the parser if we're rebasing or not.
Reported-by: Stefan Haller <lists@haller-berlin.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
rebase -i: improve error message when picking merge
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1672
builtin/rebase.c | 2 +-
rebase-interactive.c | 7 ++---
sequencer.c | 49 ++++++++++++++++++++++++++++++-----
sequencer.h | 2 +-
t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++
5 files changed, 81 insertions(+), 12 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b086f651a6..a33e41c44da 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -297,7 +297,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
else {
discard_index(&the_index);
if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
- &todo_list))
+ &todo_list, 1))
BUG("unusable todo list");
ret = complete_action(the_repository, &replay, flags,
diff --git a/rebase-interactive.c b/rebase-interactive.c
index d9718409b3d..78d5ed1a41d 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -114,7 +114,8 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
* it. If there is an error, we do not return, because the user
* might want to fix it in the first place. */
if (!initial)
- incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |
+ incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf,
+ todo_list, 1) |
file_exists(rebase_path_dropped());
if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
@@ -134,7 +135,7 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
if (initial && new_todo->buf.len == 0)
return -3;
- if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
+ if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo, 1)) {
fprintf(stderr, _(edit_todo_list_advice));
return -4;
}
@@ -234,7 +235,7 @@ int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_
int res = 0;
if (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {
- todo_list_parse_insn_buffer(r, backup.buf.buf, &backup);
+ todo_list_parse_insn_buffer(r, backup.buf.buf, &backup, 1);
res = todo_list_check(&backup, todo_list);
}
diff --git a/sequencer.c b/sequencer.c
index 91de546b323..cf808c24d20 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2550,8 +2550,37 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)
return 0;
}
+static int error_merge_commit(enum todo_command command)
+{
+ switch(command) {
+ case TODO_PICK:
+ return error(_("'%s' does not accept merge commits, "
+ "please use '%s'"),
+ todo_command_info[command].str, "merge -C");
+
+ case TODO_REWORD:
+ return error(_("'%s' does not accept merge commits, "
+ "please use '%s'"),
+ todo_command_info[command].str, "merge -c");
+
+ case TODO_EDIT:
+ return error(_("'%s' does not accept merge commits, "
+ "please use '%s' followed by '%s'"),
+ todo_command_info[command].str,
+ "merge -C", "break");
+
+ case TODO_FIXUP:
+ case TODO_SQUASH:
+ return error(_("cannot squash merge commit into another commit"));
+
+ default:
+ BUG("unexpected todo_command");
+ }
+}
+
static int parse_insn_line(struct repository *r, struct todo_item *item,
- const char *buf, const char *bol, char *eol)
+ const char *buf, const char *bol, char *eol,
+ int rebasing)
{
struct object_id commit_oid;
char *end_of_object_name;
@@ -2655,7 +2684,12 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
return status;
item->commit = lookup_commit_reference(r, &commit_oid);
- return item->commit ? 0 : -1;
+ if (!item->commit)
+ return -1;
+ if (rebasing && item->command != TODO_MERGE &&
+ item->commit->parents && item->commit->parents->next)
+ return error_merge_commit(item->command);
+ return 0;
}
int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)
@@ -2686,7 +2720,7 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *
}
int todo_list_parse_insn_buffer(struct repository *r, char *buf,
- struct todo_list *todo_list)
+ struct todo_list *todo_list, int rebasing)
{
struct todo_item *item;
char *p = buf, *next_p;
@@ -2704,7 +2738,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
item = append_new_todo(todo_list);
item->offset_in_buf = p - todo_list->buf.buf;
- if (parse_insn_line(r, item, buf, p, eol)) {
+ if (parse_insn_line(r, item, buf, p, eol, rebasing)) {
res = error(_("invalid line %d: %.*s"),
i, (int)(eol - p), p);
item->command = TODO_COMMENT + 1;
@@ -2852,7 +2886,8 @@ static int read_populate_todo(struct repository *r,
if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)
return -1;
- res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
+ res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list,
+ is_rebase_i(opts));
if (res) {
if (is_rebase_i(opts))
return error(_("please fix this using "
@@ -2882,7 +2917,7 @@ static int read_populate_todo(struct repository *r,
struct todo_list done = TODO_LIST_INIT;
if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
- !todo_list_parse_insn_buffer(r, done.buf.buf, &done))
+ !todo_list_parse_insn_buffer(r, done.buf.buf, &done, 1))
todo_list->done_nr = count_commands(&done);
else
todo_list->done_nr = 0;
@@ -6286,7 +6321,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
strbuf_release(&buf2);
/* Nothing is done yet, and we're reparsing, so let's reset the count */
new_todo.total_nr = 0;
- if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
+ if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo, 1) < 0)
BUG("invalid todo list after expanding IDs:\n%s",
new_todo.buf.buf);
diff --git a/sequencer.h b/sequencer.h
index dcef7bb99c0..ed2c4b38514 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -136,7 +136,7 @@ struct todo_list {
}
int todo_list_parse_insn_buffer(struct repository *r, char *buf,
- struct todo_list *todo_list);
+ struct todo_list *todo_list, int rebasing);
int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
const char *file, const char *shortrevisions,
const char *shortonto, int num, unsigned flags);
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 64b641002e4..20b8589ad07 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -2203,6 +2203,39 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' '
test_path_is_missing execed
'
+test_expect_success 'non-merge commands reject merge commits' '
+ test_when_finished "test_might_fail git rebase --abort" &&
+ git checkout E &&
+ git merge I &&
+ oid=$(git rev-parse HEAD) &&
+ cat >todo <<-EOF &&
+ pick $oid
+ reword $oid
+ edit $oid
+ fixup $oid
+ squash $oid
+ EOF
+ (
+ set_replace_editor todo &&
+ test_must_fail git rebase -i HEAD 2>actual
+ ) &&
+ cat >expect <<-EOF &&
+ error: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ}
+ error: invalid line 1: pick $oid
+ error: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ}
+ error: invalid line 2: reword $oid
+ error: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}
+ error: invalid line 3: edit $oid
+ error: cannot squash merge commit into another commit
+ error: invalid line 4: fixup $oid
+ error: cannot squash merge commit into another commit
+ error: invalid line 5: squash $oid
+ You can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.
+ Or you can abort the rebase with ${SQ}git rebase --abort${SQ}.
+ EOF
+ test_cmp expect actual
+'
+
# This must be the last test in this file
test_expect_success '$EDITOR and friends are unchanged' '
test_editor_unchanged
base-commit: c875e0b8e036c12cfbf6531962108a063c7a821c
--
gitgitgadget
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox