* Re: [Outreachy] Move existing tests to a unit testing framework
From: Christian Couder @ 2023-10-25 8:18 UTC (permalink / raw)
To: Achu Luma; +Cc: git, Junio C Hamano
In-Reply-To: <CAFR+8DzdFbwaiHtZSdLMqWYWh=fK0WA4c48+eBug-ZeAgddhcQ@mail.gmail.com>
On Tue, Oct 24, 2023 at 4:25 PM Achu Luma <ach.lumap@gmail.com> wrote:
>
> On Mon, Oct 23, 2023 at 2:41 PM Christian Couder
> <christian.couder@gmail.com> wrote:
> > Maybe if you have time you could add some descriptions or comments
> > related to the above emails and documents. For example you could tell
> > what the new unit test framework will be like, how the unit tests will
> > look like, etc. Maybe a short overview would be nice.
> >
> sure,
> 1- https://lore.kernel.org/git/0169ce6fb9ccafc089b74ae406db0d1a8ff8ac65.1688165272.git.steadmon@google.com/
> :
> The emails highlight the significant milestones achieved in
> defining and testing the custom TAP
> framework for writing git unit tests. It also contains some
> examples of implementation such as
> that of STRBUF_INIT with output:
> ok 1 - static initialization works
> 1..1
>
> 2- https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc:
> From this technical doc, the new unit test framework in the Git
> project represents a significant
> enhancement, introducing a systematic and efficient approach to
> unit testing. The custom git
> TAP implementation was selected from several alternatives based
> on strict criteria as the most
> suitable test framework for porting the unit tests.
> The unit tests are written in pure C, eliminating the need for
> the previous shell/test-tool helper
> setup, simplifying test configuration, data handling, and
> reducing testing runtime.
> Each unit test is encapsulated as a function and employs a range
> of predefined check functions
> for validation. These checks can evaluate conditions, compare
> integers or characters, validate
> strings, and more, providing comprehensive coverage for test scenarios.
>
> When a test is run using the TEST() macro, it undergoes a series
> of checks, and if any check fails,
> a diagnostic message is printed to aid in debugging. This
> diagnostic output includes information
> about the specific check that failed, the file and line number
> where it occurred, and a clear comparison
> of the expected and actual values. Such detailed reporting
> simplifies the identification and resolution
> of issues, contributing to codebase stability.
>
> Additionally, the framework supports features like skipping tests
> with explanations, sending custom
> diagnostic messages using test_msg(), and marking known-to-fail
> checks using TEST_TODO().
> This flexibility allows developers to tailor their tests to
> specific scenarios while ensuring a
> comprehensive testing suite.
Ok, please add all these explanations above as well as those below to
your application document. We prefer that you consider your
application document like a patch. So you would send to the mailing
list several versions of it for review before submitting officially.
> > You could also try to apply the patches in the series that adds the
> > test framework, or alternatively use the 'seen' branch where the
> > series has been merged, and start playing with it by writing, or
> > porting, a small example test.
> >
> ok, I think I can push a patch for one.
I don't think it's necessary to send it to the mailing list for now,
but it should definitely be part of your application document.
> > > -- Create a New C Test File: For each unit test I plan to migrate, create a new C source file (.c) in the Git project's test suite directory(t/unit-tests). Name it appropriately to reflect the purpose of the test.
> >
> > Could you provide an example of what the new name would be for an
> > existing test that is worth porting?
> >
> Sure... let's consider an existing unit test in t/helper directory
> such as t/helper/test-date.c or
> its shell named t0006-date.sh, which is part of the current
> shell-based test suite. In the context
> of the new unit testing framework, this test could be reimagined and renamed as
> "t-date.c". The "t-" prefix is typically used for test program files
> in Git, and "date" is retained to
> reflect the nature of the tests within this suite.
Ok.
> > > -- Include Necessary Headers:In the new C test file, include the necessary Git unit test framework headers. Typically, this includes headers like "test-lib.h" and others relevant to the specific test.
> > > #include "test-lib.h"
> >
> > Maybe you could continue the above example and tell which headers
> > would be needed for it?
> >
> > > -- Convert Test Logic: Refactor the test logic from the original Shell script into the new C-based test format. Use the testing macros provided by the Git unit test framework, such as test_expect_success, test_expect_failure, etc., to define the tests.
> > > test_expect_success("simple progress display", "{
> > > // Test logic here...
> > > }");
> >
> > Ok, a simple example would be nice too.
> >
> we can continue with the example used for naming: test-date.c. a
> typical t-date.c unit test would look
> like the following:
> --
> #include "test-lib.h"
> #include "date.h"
> --
> date.h here is a necessary header file. Now refactoring the test logic
> from the original shell script:
> --
> #include "test-lib.h"
> #include "date.h"
>
> static void test_parse_dates(void)
> {
> const char *dates[] = { "invalid_date", "2023-10-17 10:00:00 +0200", NULL };
>
> for (const char **argv = dates; *argv; argv++) {
> check_int(parse_dates((const char *[]){ *argv, NULL }), 0);
> }
> }
> --
Nice!
> > > -- Add Test Descriptions: Provide clear and informative descriptions for each test using the testing macros. These descriptions will help in identifying the purpose of each test when the test suite is run.
> >
> > This would seem to be part of the previous step, as you would have to
> > provide a description when using the testing macro. But Ok.
> >
> > > -- Define a Test Entry Point: Create a cmd_main function as the entry point for the C-based tests. Inside this function, include the test functions using the testing macros.
> > > int cmd_main(int argc, const char **argv) {
> > > // Test functions...
> > > return test_done();
> > > }
> >
> > Yeah, continuing an example would be nice.
> >
> Continuing, we can add a test entrance as follows:
> --
> #include "test-lib.h"
> #include "date.h"
>
> static void test_parse_dates(void)
> {
> const char *dates[] = { "invalid_date", "2023-10-17 10:00:00 +0200", NULL };
>
> for (const char **argv = dates; *argv; argv++) {
> check_int(parse_dates((const char *[]){ *argv, NULL }), 0);
> }
> }
>
> int main(int argc UNUSED, const char **argv UNUSED)
> {
> TEST(test_parse_dates, "Test date parsing");
>
> return test_done();
> }
> --
>
> A typical unit tests with the custom TAP framework would look
> something like above. This might run in theory
> but I have not yet run it as I used it here just for demonstration.
> The unit tests can be built using
> "make unit-tests." Additionally, Makefile can be modified to add the
> file to the build:
> --
> UNIT_TEST_PROGRAMS += t-date
> --
Great!
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH 2/2] fetch: no redundant error message for atomic fetch
From: Patrick Steinhardt @ 2023-10-25 8:21 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <CANYiYbG0YFc4Hg=e+0db4NBgM2QwOLpjHjfp8WaoObNxR-=euA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3244 bytes --]
On Tue, Oct 24, 2023 at 07:20:08AM +0800, Jiang Xin wrote:
> On Mon, Oct 23, 2023 at 6:07 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Mon, Oct 23, 2023 at 05:16:20PM +0800, Jiang Xin wrote:
> > > On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt <ps@pks.im> wrote:
> > > >
> > > > On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:
> > > > > @@ -1775,10 +1775,8 @@ static int do_fetch(struct transport *transport,
> > > > > }
> > > > >
> > > > > cleanup:
> > > > > - if (retcode && transaction) {
> > > > > - ref_transaction_abort(transaction, &err);
> > > > > + if (retcode && transaction && ref_transaction_abort(transaction, &err))
> > > > > error("%s", err.buf);
> > > > > - }
> > > >
> > > > Right. We already call `error()` in all cases where `err` was populated
> > > > before we `goto cleanup;`, so calling it unconditionally a second time
> > > > here is wrong.
> > > >
> > > > That being said, `ref_transaction_abort()` will end up calling the
> > > > respective backend's implementation of `transaction_abort`, and for the
> > > > files backend it actually ignores `err` completely. So if the abort
> > > > fails, we would still end up calling `error()` with an empty string.
> > >
> > > The transaction_abort implementations of the two builtin refs backends
> > > will not use "err“ because they never fail (always return 0). Some one
> > > may want to implement their own refs backend which may use the "err"
> > > variable in their "transaction_abort". So follow the pattern as
> > > update-ref.c and files-backend.c to call ref_transaction_abort() is
> > > safe.
> > >
> > > > Furthermore, it can happen that `transaction_commit` fails, writes to
> > > > the buffer and then prints the error. If the abort now fails as well, we
> > > > would end up printing the error message twice.
> > >
> > > The abort never fails so error message from transaction_commit() will
> > > not reach the code.
> >
> > With that reasoning we could get rid of the error handling of abort
> > completely as it's known not to fail. But only because it does not fail
> > right now doesn't mean that it won't in the future, as the infra for it
> > to fail is all in place. And in case it ever does the current code will
> > run into the bug I described.
>
> If in the future ref_transaction_abort() fails for some reason, the
> err variable will be filled with the error message and the previous
> error message will be discarded, no duplication will occur. So I think
> use this fix is OK.
Isn't that assuming quite a lot about that future code though? It
assumes both:
- That the code knows to always populate the error in the first
place. Otherwise we may end up with the same empty error message
that you aim to fix.
- That the code will know to overwrite it instead of appending to
it. Otherwise we may end up printing previous errors a second
time.
Both of these assumptions may not hold. Current code that does write to
the error buffer for example will always append to it, not overwrite its
preexisting contents. And it's likely that future code will do the same.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 0/2] pretty: add %I formatting for patch-id
From: Jeff King @ 2023-10-25 8:44 UTC (permalink / raw)
To: Michael McClimon; +Cc: git
In-Reply-To: <20231022022800.69219-1-michael@mcclimon.org>
On Sat, Oct 21, 2023 at 10:27:58PM -0400, Michael McClimon wrote:
> I would like to have a single-command way to get the patch id for a
> commit: the thing you'd see in a pipeline like
> git diff-tree --patch-with-raw HEAD | git patch-id
So I actually don't think this is the worst thing in the world. You are
using two plumbing commands as intended. And if you want to get N
patch-ids, you can do it with a constant number of processes like:
git rev-list HEAD | git diff-tree --stdin -p | git patch-id
Alternatively, I wonder if patch-id could generate the ids internally
(which we certainly have code for; that's how git-cherry, etc, work).
Would it make sense to teach patch-id:
1. To accept just commit id's on stdin to produce the patches for
those commit id's against their parents, without receiving the
patch text itself on stdin. I guess --commits-only or something.
2. To accept commit-ishes on the command line, overriding the need to
read from stdin.
Which would let your single-patch case become just:
git patch-id --commits-only HEAD
But let's read on...
> My initial thought was to add a --patch-id flag to git diff-tree, but
> then I thought that maybe better would be to add a pretty specifier to
> do so, so that (for instance) you could generate patch-ids for
> everything in a branch by saying something like
> git log --pretty='%I %H' start..
I can see how "%I" would be convenient, especially from a scripting
standpoint, since you can control how it appears in the output. But it
does feel a little funny to me as a commit pretty-printing item.
Computing it requires calculating the diff, which we'll of course want
to do separately for the diff portion of many invocations. E.g., I
didn't check, but I'd guess that:
git log --pretty="%I %H" -p
would compute each diff twice. Would it make more sense as a
diff-format alongside --raw, --name-status, and so on?
I have a more subtle reason to think this might be a good idea, which
I'll discuss below.
> The thing that is perplexing to me is that it _does_ appear to work on
> some commits, for example (where 8b3aa36f is a recent-ish commit from
> master, chosen at random):
> [...]
> But for other commits, like the one in the test here, it does not.
Hmm. When I run the failing test in a debugger, I see that going into
commit_patch_id(), there is already an entry in diff_queued_diff (which
is a global that stores the result of the last diff). So I think there
is already a diff there from the call to diff_tree_oid() in
log_tree_diff(). And then you diff _again_ and end up with duplicates
when computing the patch-id.
(This causes another bug, too; at the end of diff_flush_patch_id we
clear the queue, so the actual diff is not showed by "git show" anymore;
the use of "-s" in the test obscures that).
The thing that most surprises me is that it ever works at all. ;) But I
think what happens is this:
- in the code in your patch, you fail to set up the diff_options
correctly. In particular, you do not set the "recursive" flag.
- so in the case of 8b3aa36f, which touches only a file in
Documentation/, the call from log_tree_diff() queues the correct
file modification entry. The second call from commit_patch_id()
mistakenly queues only the parent tree "Documentation"
- a patch-id is defined on the textual patch changes, so it ignores
non-file changes that are queued (and in Git you wouldn't normally
see these anyway unless you use a non-recursive diff or a special
option like "-t").
And hence any patch which consists of changes to files only in subtrees
will "work", and other ones fail. To "fix" your patch, you could set
diffopt.flags.recursive, as we do in format-patch's prepare_bases()
call. But of course because of the duplicate diff, that just means we'll
consistently get the wrong answer now. ;)
So I'd expect something like this to work:
diff --git a/pretty.c b/pretty.c
index 47e2e6e99a..5a473c7941 100644
--- a/pretty.c
+++ b/pretty.c
@@ -20,6 +20,7 @@
#include "run-command.h"
#include "object-name.h"
#include "patch-ids.h"
+#include "diffcore.h"
/*
* The limit for formatting directives, which enable the caller to append
@@ -1576,10 +1577,28 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
{
struct diff_options diffopt;
struct object_id patch_id;
+
+ /*
+ * save the existing diff queue, as it may be
+ * populated, e.g. by log_tree_diff(). Of course
+ * it would be nice to use its results, but:
+ *
+ * 1. We can't count on it (we might be formatting a
+ * via some other code path).
+ * 2. We have no clue which diff options were used.
+ */
+ struct diff_queue_struct old_queue = diff_queued_diff;
+ DIFF_QUEUE_CLEAR(&diff_queued_diff);
+
repo_diff_setup(the_repository, &diffopt);
+ diffopt.flags.recursive = 1;
+ diff_setup_done(&diffopt);
+
if (commit_patch_id(commit, &diffopt, &patch_id, 0))
die(_("cannot get patch id"));
strbuf_addstr(sb, oid_to_hex(&patch_id));
+
+ diff_queued_diff = old_queue;
return 1;
}
case 'm': /* left/right/bottom */
but it doesn't quite fix your test. There I think the issue is that
the patch-id code is a bit wonky for the special case of the creation of
an empty file (even before your patch). The internal patch-id generator
adds fake:
--- /dev/null
+++ b/whatever
lines that would normally appear in the patch for a newly created file.
But for an empty file, the "--patch" format does not emit those lines at
all! So we need this hackery on top:
diff --git a/diff.c b/diff.c
index 2c602df10a..9370e585f1 100644
--- a/diff.c
+++ b/diff.c
@@ -6431,15 +6431,15 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid
the_hash_algo->update_fn(&ctx, oid_to_hex(&p->two->oid),
the_hash_algo->hexsz);
} else {
- if (p->one->mode == 0) {
+ if (p->one->mode == 0 && p->two->size) {
patch_id_add_string(&ctx, "---/dev/null");
patch_id_add_string(&ctx, "+++b/");
the_hash_algo->update_fn(&ctx, p->two->path, len2);
- } else if (p->two->mode == 0) {
+ } else if (p->two->mode == 0 && p->one->size) {
patch_id_add_string(&ctx, "---a/");
the_hash_algo->update_fn(&ctx, p->one->path, len1);
patch_id_add_string(&ctx, "+++/dev/null");
- } else {
+ } else if (p->one->size || p->two->size) {
patch_id_add_string(&ctx, "---a/");
the_hash_algo->update_fn(&ctx, p->one->path, len1);
patch_id_add_string(&ctx, "+++b/");
And with that, your new test succeeds.
> have done a bit of investigation, but I would not really call myself a C
> programmer and I'm not super familiar with the codebase, so I'm a bit
> stuck. I thought maybe at first I wasn't initializing the diff_options
> correctly, but I suspect the problem is actually more fundamental than
> that.
So as you can see, the diff options do matter. And that is a more
fundamental issue. In many cases patch-id will be pretty stable and
independent of options, but I suspect it would get confused by things
like rename detection.
If we run "git log --format=%I", which diff options should be used for
the patch-id? A stable set (like we do for "format-patch --base")? Or
ones that match the diff we are otherwise generating? I could see
arguments both ways.
One nice thing about "diff-tree | patch-id" is that you are in control
of the options that diff-tree uses. But I'd think in most cases you just
want a stable patch-id, so the "git patch-id HEAD" I suggested earlier
would presumably use some very basic set of diff options.
I dunno. I do not use patch-id much (aside from internal use via things
like "git cherry", etc). So there may be use cases or corner cases I'm
not thinking about. But I do think there's a bit of complexity to think
about here.
-Peff
^ permalink raw reply related
* [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Ruslan Yakauleu via GitGitGadget @ 2023-10-25 8:58 UTC (permalink / raw)
To: git; +Cc: Ruslan Yakauleu, Ruslan Yakauleu
From: Ruslan Yakauleu <ruslan.yakauleu@gmail.com>
A new option --ff-one-only to control the merging strategy.
For one commit option works like -ff to avoid extra merge commit.
In other cases the option works like --no-ff to create merge commit for
complex features.
Signed-off-by: Ruslan Yakauleu <ruslan.yakauleu@gmail.com>
---
merge: --ff-one-only to apply FF if commit is one
A new option --ff-one-only to control the merging strategy. For one
commit option works like -ff to avoid extra merge commit. In other cases
the option works like --no-ff to create merge commit for complex
features.
Plenty of developers want to simplify merge history. We have two main
merging strategies:
* Fast-forward (--ff) - There we can lose merge commits for complex
features and if we need to roll back some feature we can't revert
just one commit.
* Merge (--no-ff) - There we have extra merges for extra simple
changes.
Before, the user had to choose between --ff/--no-ff every time to have
history without extra merges for simple changes and to use merges for
complex features.
Ways to use the new option: git merge --ff-one-only git config merge.ff
one-only git config branch.master.mergeoptions --ff-one-only
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1599%2FQuAzI%2Fmerge%2Fff-one-only-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1599/QuAzI/merge/ff-one-only-v1
Pull-Request: https://github.com/git/git/pull/1599
Documentation/config/merge.txt | 3 +++
builtin/merge.c | 18 +++++++++++++++++-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/Documentation/config/merge.txt b/Documentation/config/merge.txt
index 8851b6cedef..6cd5daa4d64 100644
--- a/Documentation/config/merge.txt
+++ b/Documentation/config/merge.txt
@@ -31,6 +31,9 @@ merge.ff::
a case (equivalent to giving the `--no-ff` option from the command
line). When set to `only`, only such fast-forward merges are
allowed (equivalent to giving the `--ff-only` option from the
+ command line). When set to `one-only`, fast-forward merge allowed
+ only for one commit, in other way extra merge commit should be
+ created (equivalent to giving the `--ff-one-only` option from the
command line).
merge.verifySignatures::
diff --git a/builtin/merge.c b/builtin/merge.c
index d748d46e135..100f0021e56 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -110,7 +110,8 @@ static const char *pull_twohead, *pull_octopus;
enum ff_type {
FF_NO,
FF_ALLOW,
- FF_ONLY
+ FF_ONLY,
+ FF_ONE_ONLY
};
static enum ff_type fast_forward = FF_ALLOW;
@@ -258,6 +259,7 @@ static struct option builtin_merge_options[] = {
N_("edit message before committing")),
OPT_CLEANUP(&cleanup_arg),
OPT_SET_INT(0, "ff", &fast_forward, N_("allow fast-forward (default)"), FF_ALLOW),
+ OPT_SET_INT(0, "ff-one-only", &fast_forward, N_("allow fast-forward if only one commit"), FF_ONE_ONLY),
OPT_SET_INT_F(0, "ff-only", &fast_forward,
N_("abort if fast-forward is not possible"),
FF_ONLY, PARSE_OPT_NONEG),
@@ -631,6 +633,8 @@ static int git_merge_config(const char *k, const char *v,
fast_forward = boolval ? FF_ALLOW : FF_NO;
} else if (v && !strcmp(v, "only")) {
fast_forward = FF_ONLY;
+ } else if (v && !strcmp(v, "one-only")) {
+ fast_forward = FF_ONE_ONLY;
} /* do not barf on values from future versions of git */
return 0;
} else if (!strcmp(k, "merge.defaulttoupstream")) {
@@ -1527,6 +1531,18 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
free(list);
}
+ if (fast_forward == FF_ONE_ONLY) {
+ fast_forward = FF_NO;
+
+ /* check that we have one and only one commit to merge */
+ if (squash || ((!remoteheads->next &&
+ !common->next &&
+ oideq(&common->item->object.oid, &head_commit->object.oid)) &&
+ oideq(&remoteheads->item->parents->item->object.oid, &head_commit->object.oid))) {
+ fast_forward = FF_ALLOW;
+ }
+ }
+
update_ref("updating ORIG_HEAD", "ORIG_HEAD",
&head_commit->object.oid, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
base-commit: 2e8e77cbac8ac17f94eee2087187fa1718e38b14
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Oswald Buddenhagen @ 2023-10-25 9:23 UTC (permalink / raw)
To: Jeff King
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
In-Reply-To: <20231025061120.GA2094463@coredump.intra.peff.net>
On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:
>The "//" operator was added in perl 5.10. I'm not sure what you found
>that makes you think the ship has sailed. The only hits for "//" I see
>look like the end of substitution regexes ("s/foo//" and similar).
>
grep with spaces around the operator, then you can see the instance in
git-credential-netrc.perl easily.
regards
^ permalink raw reply
* Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option
From: Karthik Nayak @ 2023-10-25 9:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ps
In-Reply-To: <xmqqbkcn52z4.fsf@gitster.g>
On Tue, Oct 24, 2023 at 7:45 PM Junio C Hamano <gitster@pobox.com> wrote:
> I would suspect that swapping if/else would make it easier to
> follow. Everybody else in the patch guards the use of the oidset
> with "were we told not to die on missing objects?", i.e.,
>
> if (revs->do_not_die_on_missing_objects)
> oidset_insert(&revs->missing_objects, &p->object.oid);
> else
> return -1; /* corrupt repository */
>
Makes sense. Will change.
> > @@ -3800,6 +3809,9 @@ int prepare_revision_walk(struct rev_info *revs)
> > FOR_EACH_OBJECT_PROMISOR_ONLY);
> > }
> >
> > + if (revs->do_not_die_on_missing_objects)
> > + oidset_init(&revs->missing_objects, 0);
>
> I read the patch to make sure that .missing_objects oidset is used
> only when .do_not_die_on_missing_objects is set and the oidset is
> untouched unless it is initialized. Well done.
>
> I know I floated "perhaps oidset can replace the object bits,
> especially because the number of objects that need marking is
> expected to be small", but I am curious what the performance
> implication of this would be. Is this something we can create
> comparison easily?
I did a simple comparision here, I randomly deleted commits which had
child commits with greater than 2 parents.
$ git rev-list --missing=print HEAD | grep "?" | wc -l
828
Using the flag bit:
$ hyperfine -w 3 "~/git/bin-wrappers/git rev-list --missing=allow-any HEAD"
Benchmark 1: ~/git/bin-wrappers/git rev-list --missing=allow-any HEAD
Time (mean ± σ): 860.5 ms ± 15.2 ms [User: 375.3 ms, System: 467.5 ms]
Range (min … max): 835.2 ms … 881.0 ms 10 runs
Using the oidset:
$ hyperfine -w 3 "~/git/bin-wrappers/git rev-list --missing=allow-any HEAD"
Benchmark 1: ~/git/bin-wrappers/git rev-list --missing=allow-any HEAD
Time (mean ± σ): 901.3 ms ± 9.6 ms [User: 394.3 ms, System: 488.3 ms]
Range (min … max): 885.0 ms … 913.2 ms 10 runs
Its definitely slower, but not by much.
>
> > I noticed that nobody releases the resource held by this new oidset.
> > Shouldn't we do so in revision.c:release_revisions()?
>
> It seems that linux-leaks CI job noticed the same.
>
> https://github.com/git/git/actions/runs/6633599458/job/18021612518#step:5:2949
>
> I wonder if the following is sufficient?
>
> revision.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git c/revision.c w/revision.c
> index 724a116401..7a67ff74dc 100644
> --- c/revision.c
> +++ w/revision.c
> @@ -3136,6 +3136,8 @@ void release_revisions(struct rev_info *revs)
> clear_decoration(&revs->merge_simplification, free);
> clear_decoration(&revs->treesame, free);
> line_log_free(revs);
> + if (revs->do_not_die_on_missing_objects)
> + oidset_clear(&revs->missing_objects);
> }
>
> static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
>
Yup, this seems right and was missed, will add.
On Wed, Oct 25, 2023 at 8:40 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> >
> > if (commit->object.flags & ADDED)
> > return 0;
> > + if (revs->do_not_die_on_missing_objects &&
> > + oidset_contains(&revs->missing_objects, &commit->object.oid))
>
> Nit: indentation is off here.
>
Thanks, will fix.
> > +
> > + /* Missing objects to be tracked without failing traversal. */
> > + struct oidset missing_objects;
>
> As far as I can see we only use this set to track missing commits, but
> none of the other objects. The name thus feels a bit misleading to me,
> as a reader might rightfully assume that it contains _all_ missing
> objects after the revwalk. Should we rename it to `missing_commits` to
> clarify?
>
Fair enough, I was thinking of being future compatible. But probably best to
be specific now and refactor as needed in the future. Will change.
Thanks both for the review!
^ permalink raw reply
* [PATCH v3] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Oswald Buddenhagen @ 2023-10-25 10:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Phillip Wood, Taylor Blau, Christian Couder,
Charvi Mendiratta, Marc Branchaud
In-Reply-To: <20231023130016.1093356-1-oswald.buddenhagen@gmx.de>
Create a clear top-down structure which makes it hopefully unambiguous
what happens when.
The behavior in the presence of multiple "fixup -c" is somewhat
questionable, as arguably it would be better to complain about it rather
than letting the last instance win. But for the time being we document
the status quo, with a note that it is not guaranteed. Note that
actually changing it would require --autosquash eliding the superseded
uses.
Also emphasize that the author info of the first commit is preserved
even in the presence of "fixup -c", as this diverges from "git commit
-c"'s behavior. New options matching the latter should be introduced for
completeness.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v3:
- adjust to reality, and elaborate in the commit message why it's
arguably somewhat suboptimal
i deliberated the 'command "pick"' word order swap suggested by marc,
but while it improves things locally, it somehow doesn't flow with the
"redundancy-reduced" last part of the sentence.
v2:
- slight adjustments inspired by marc. however, i left most things
unchanged or even went in the opposite direction, because i assume the
readers to be sufficiently context-sensitive, and the objective is
merely to be not actively confusing. adding redundancy in the name of
clarity would just make the text stylistically inferior and arguably
harder to read.
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Phillip Wood <phillip.wood123@gmail.com>
Cc: Taylor Blau <me@ttaylorr.com>
Cc: Christian Couder <christian.couder@gmail.com>
Cc: Charvi Mendiratta <charvi077@gmail.com>
Cc: Marc Branchaud <marcnarc@xiplink.com>
---
Documentation/git-rebase.txt | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index e7b39ad244..578d1d34a6 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -890,20 +890,22 @@ command "pick" with the command "reword".
To drop a commit, replace the command "pick" with "drop", or just
delete the matching line.
-If you want to fold two or more commits into one, replace the command
-"pick" for the second and subsequent commits with "squash" or "fixup".
-If the commits had different authors, the folded commit will be
-attributed to the author of the first commit. The suggested commit
-message for the folded commit is the concatenation of the first
-commit's message with those identified by "squash" commands, omitting the
-messages of commits identified by "fixup" commands, unless "fixup -c"
-is used. In that case the suggested commit message is only the message
-of the "fixup -c" commit, and an editor is opened allowing you to edit
-the message. The contents (patch) of the "fixup -c" commit are still
-incorporated into the folded commit. If there is more than one "fixup -c"
-commit, the message from the final one is used. You can also use
-"fixup -C" to get the same behavior as "fixup -c" except without opening
-an editor.
+If you want to fold two or more commits into one (that is, to combine
+their contents/patches), replace the command "pick" for the second and
+subsequent commits with "squash" or "fixup".
+The commit message for the folded commit is the concatenation of the
+message of the first commit with those of commits identified by "squash"
+commands, omitting those of commits identified by "fixup" commands,
+unless "fixup -c" is used. In the latter case, the message is obtained
+only from the "fixup -c" commit (if multiple are present, the last one
+takes precedence, but this should not be relied upon).
+If the resulting commit message is a concatenation of multiple messages,
+an editor is opened allowing you to edit it. This is also the case for a
+message obtained via "fixup -c", while using "fixup -C" instead skips
+the editor; this is analogous to the behavior of `git commit`.
+The author information (including date/timestamp) always comes from
+the first commit; this is the case even if "fixup -c/-C" is used,
+contrary to what `git commit` does.
`git rebase` will stop when "pick" has been replaced with "edit" or
when a command fails due to merge errors. When you are done editing
--
2.42.0.419.g70bf8a5751
^ permalink raw reply related
* Re: [PATCH 05/12] builtin/show-ref: refactor `--exclude-existing` options
From: Patrick Steinhardt @ 2023-10-25 11:50 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <CAPig+cQrD6uh65UzaKbwryv=wcdymKrjqXsAKgrKHYpQNWqSYQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3171 bytes --]
On Tue, Oct 24, 2023 at 02:48:14PM -0400, Eric Sunshine wrote:
> On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote:
> > It's not immediately obvious options which options are applicable to
> > what subcommand ni git-show-ref(1) because all options exist as global
>
> s/ni/in/
>
> > state. This can easily cause confusion for the reader.
> >
> > Refactor options for the `--exclude-existing` subcommand to be contained
> > in a separate structure. This structure is stored on the stack and
> > passed down as required. Consequentially, it clearly delimits the scope
>
> s/Consequentially/Consequently/
>
> > of those options and requires the reader to worry less about global
> > state.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> > @@ -95,6 +94,11 @@ static int add_existing(const char *refname,
> > +struct exclude_existing_options {
> > + int enabled;
> > + const char *pattern;
> > +};
>
> Do we need this `enabled` flag? Can't the same be achieved by checking
> whether `pattern` is NULL or not (see below)?
Yeah, we do. It's perfectly valid to pass `--exclude-existing` without
the optional pattern argument. We still want to use this mode in that
case, but don't populate the pattern.
An alternative would be to assign something like a sentinel value in
here. But I'd think that it's clearer to instead have an explicit
separate field for this.
> > @@ -104,11 +108,11 @@ static int add_existing(const char *refname,
> > -static int cmd_show_ref__exclude_existing(const char *match)
> > +static int cmd_show_ref__exclude_existing(const struct exclude_existing_options *opts)
>
> Since you're renaming `match` to `opts->pattern`...
>
> > {
> > - int matchlen = match ? strlen(match) : 0;
> > + int matchlen = opts->pattern ? strlen(opts->pattern) : 0;
>
> ... and since you're touching this line anyway, maybe it makes sense
> to rename `matchlen` to `patternlen`?
Yes, let's do it. It's been more of an oversight rather than
intentional to keep the previous name.
> > @@ -124,11 +128,11 @@ static int cmd_show_ref__exclude_existing(const char *match)
> > - if (strncmp(ref, match, matchlen))
> > + if (strncmp(ref, opts->pattern, matchlen))
>
> Especially since, as shown in this context, `matchlen` is really the
> length of the _pattern_, not the length of the resulting _match_.
>
> > @@ -200,44 +204,46 @@ static int hash_callback(const struct option *opt, const char *arg, int unset)
> > int cmd_show_ref(int argc, const char **argv, const char *prefix)
> > {
> > [...]
> > - if (exclude_arg)
> > - return cmd_show_ref__exclude_existing(exclude_existing_arg);
> > + if (exclude_existing_opts.enabled)
> > + return cmd_show_ref__exclude_existing(&exclude_existing_opts);
>
> (continued from above) Can't this be handled without a separate `enabled` flag?
>
> if (exclude_existing_opts.pattern)
> ...
See the explanation above.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 10/12] builtin/show-ref: explicitly spell out different modes in synopsis
From: Patrick Steinhardt @ 2023-10-25 11:50 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <CAPig+cQ=VhRm40oW=TQYzy2NXKNJm4QVQhtw3FAKsFRn12qYLA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3246 bytes --]
On Tue, Oct 24, 2023 at 03:39:28PM -0400, Eric Sunshine wrote:
> On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote:
> > The synopsis treats the `--verify` and the implicit mode the same. They
> > are slightly different though:
> >
> > - They accept different sets of flags.
> >
> > - The implicit mode accepts patterns while the `--verify` mode
> > accepts references.
> >
> > Split up the synopsis for these two modes such that we can disambiguate
> > those differences.
>
> Good. When reading [2/12], my immediate thought was that such a
> documentation change was needed.
>
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
> > @@ -8,9 +8,12 @@ git-show-ref - List references in a local repository
> > SYNOPSIS
> > -'git show-ref' [-q | --quiet] [--verify] [--head] [-d | --dereference]
> > +'git show-ref' [-q | --quiet] [--head] [-d | --dereference]
> > [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]
> > [--heads] [--] [<pattern>...]
> > +'git show-ref' --verify [-q | --quiet] [-d | --dereference]
> > + [-s | --hash[=<n>]] [--abbrev[=<n>]]
> > + [--] [<ref>...]
> > 'git show-ref' --exclude-existing[=<pattern>]
>
> What does it mean to request "quiet" for the plain `git show-ref`
> mode? That seems pointless and counterintuitive. Even though this mode
> may accept --quiet as a quirk of implementation, we probably shouldn't
> be promoting its use in the documentation. Moreover, the blurb for
> --quiet later in the document:
>
> Do not print any results to stdout. When combined with --verify,
> this can be used to silently check if a reference exists.
>
> should probably be rephrased since it currently implies that it may be
> used with modes other than --verify, but that's not really the case
> (implementation quirks aside).
Good point indeed, will change.
> This also raises the question as to whether an interlock should be
> added to disallow --quiet with plain `git show-ref`, much like the
> interlock preventing --exclude-existing and --verify from being used
> together. Ideally, such an interlock ought to be added, but I wouldn't
> be surprised to learn that doing so would break someone's existing
> tooling which insensibly uses --quiet with plain `git show-ref`.
Yeah, I also wouldn't go as far as this. The mutual exclusiveness for
`--exclude-existing` and `--verify` makes sense in my opinion because
the result is extremely misleading and may cause users to assume that
the wrong thing has happened.
I don't think that's necessarily true for `--quiet`. It may not make a
lot of sense to specify `--quiet` here, but it also doesn't quietly do
the wrong thing as in the other case.
Furthermore, we also don't have any interlocks for incompatible other
flags, either: git-show-ref(1) won't complain when passing any of the
mode-specific flags to the other modes. If we want to fix that I'd
rather defer it to a follow up patch series though. And as you said, I
would almost certainly expect there to be some kind of fallout if we did
this change.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 11/12] builtin/show-ref: add new mode to check for reference existence
From: Patrick Steinhardt @ 2023-10-25 11:50 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <CAPig+cRTOMie0rUf=Mhbo9e2EXf-_2kQyMeqpB9OCRB1MZZ1rw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6936 bytes --]
On Tue, Oct 24, 2023 at 05:01:55PM -0400, Eric Sunshine wrote:
> On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote:
> > While we have multiple ways to show the value of a given reference, we
> > do not have any way to check whether a reference exists at all. While
> > commands like git-rev-parse(1) or git-show-ref(1) can be used to check
> > for reference existence in case the reference resolves to something
> > sane, neither of them can be used to check for existence in some other
> > scenarios where the reference does not resolve cleanly:
> >
> > - References which have an invalid name cannot be resolved.
> >
> > - References to nonexistent objects cannot be resolved.
> >
> > - Dangling symrefs can be resolved via git-symbolic-ref(1), but this
> > requires the caller to special case existence checks depending on
> > whteher or not a reference is symbolic or direct.
>
> s/whteher/whether/
>
> > Furthermore, git-rev-list(1) and other commands do not let the caller
> > distinguish easily between an actually missing reference and a generic
> > error.
> >
> > Taken together, this gseems like sufficient motivation to introduce a
>
> s/gseems/seems/
>
> > separate plumbing command to explicitly check for the existence of a
> > reference without trying to resolve its contents.
> >
> > This new command comes in the form of `git show-ref --exists`. This
> > new mode will exit successfully when the reference exists, with a
> > specific error code of 2 when it does not exist, or with 1 when there
> > has been a generic error.
> >
> > Note that the only way to properly implement this command is by using
> > the internal `refs_read_raw_ref()` function. While the public function
> > `refs_resolve_ref_unsafe()` can be made to behave in the same way by
> > passing various flags, it does not provide any way to obtain the errno
> > with which the reference backend failed when reading the reference. As
> > such, it becomes impossible for us to distinguish generic errors from
> > the explicit case where the reference wasn't found.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
> > @@ -65,6 +70,12 @@ OPTIONS
> > +--exists::
> > +
> > + Check whether the given reference exists. Returns an error code of 0 if
>
> We probably want to call this "exit code" rather than "error code"
> since the latter is unnecessarily scary sounding for the success case
> (when the ref does exit).
I was trying to stick to the preexisting style of "error code" in this
manual page. But I think I agree with your argument that we also call it
an error code in the successful case, which is misleading.
> > + it does, 2 if it is missing, and 128 in case looking up the reference
> > + failed with an error other than the reference being missing.
>
> The commit message says it returns 1 for a generic error, but this
> inconsistently says it returns 128 for that case. The actual
> implementation returns 1.
Good catch, fixed.
> > diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> > @@ -214,6 +215,41 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts,
> > +static int cmd_show_ref__exists(const char **refs)
> > +{
> > + struct strbuf unused_referent = STRBUF_INIT;
> > + struct object_id unused_oid;
> > + unsigned int unused_type;
> > + int failure_errno = 0;
> > + const char *ref;
> > + int ret = 1;
> > +
> > + if (!refs || !*refs)
> > + die("--exists requires a reference");
> > + ref = *refs++;
> > + if (*refs)
> > + die("--exists requires exactly one reference");
> > +
> > + if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
> > + &unused_oid, &unused_referent, &unused_type,
> > + &failure_errno)) {
> > + if (failure_errno == ENOENT) {
> > + error(_("reference does not exist"));
>
> The documentation doesn't mention this printing any output, and indeed
> one would intuitively expect a boolean-like operation to not produce
> any printed output since its exit code indicates the result (except,
> of course, in the case of a real error).
I'm inclined to leave this as-is. While the exit code should be
sufficient, I think it's rather easy to wonder whether it actually did
anything at all and why it failed in more interactive use cases. Not
that I think these will necessarily exist.
I also don't think it's going to hurt to print this error. If it ever
does start to become a problem we might end up honoring the "--quiet"
flag to squelch this case.
> > + ret = 2;
> > + } else {
> > + error(_("failed to look up reference: %s"), strerror(failure_errno));
>
> Or use error_errno():
>
> errno = failure_errno;
> error_errno(_("failed to look up reference: %s"));
Ah, good suggestion.
> > + }
> > +
> > + goto out;
> > + }
> > +
> > + ret = 0;
> > +
> > +out:
> > + strbuf_release(&unused_referent);
> > + return ret;
> > +}
>
> It's a bit odd having `ret` be 1 at the outset rather than 0, thus
> making the logic a bit more difficult to reason about. I would have
> expected it to be organized like this:
>
> int ret = 0;
> if (refs_read_raw_ref(...)) {
> if (failure_errno == ENOENT) {
> ret = 2;
> } else {
> ret = 1;
> errno = failure_errno;
> error_errno(_("failed to look up reference: %s"));
> }
> }
> strbuf_release(...);
> return ret;
Fair enough. I've seen both styles used in our codebase, but ultimately
don't care much which of either we use here. Will adapt.
> > @@ -272,13 +309,15 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
> > + if ((!!exclude_existing_opts.enabled + !!verify + !!exists) > 1)
> > + die(_("only one of --exclude-existing, --exists or --verify can be given"));
>
> When reviewing an earlier patch in this series, I forgot to mention
> that we can simplify the life of translators by using placeholders:
>
> die(_("options '%s', '%s' or '%s' cannot be used together"),
> "--exclude-existing", "--exists", "--verify");
>
> which ensures that they don't translate the literal option names, and
> makes it possible to reuse the translated message in multiple
> locations (since it doesn't mention hard-coded option names).
Done.
Thanks for your review, highly appreciated! I'll wait until tomorrow for
additional feedback and then send out v2.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH] merge: ignore whitespace changes when detecting renames
From: Stefan Muenzel via GitGitGadget @ 2023-10-25 12:14 UTC (permalink / raw)
To: git; +Cc: Stefan Muenzel, Stefan Muenzel
From: Stefan Muenzel <source@s.muenzel.net>
The options ignore-space-changes and ignore-all-space for
merge strategies based on "ort" are now propagated
to the computation of file renames, by normalizing spacing
changes when computing file hashes.
Signed-off-by: Stefan Muenzel <source@s.muenzel.net>
---
merge: ignore whitespace changes when detecting renames
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1600%2Fsmuenzel%2Frenames-ignore-whitespace-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1600/smuenzel/renames-ignore-whitespace-v1
Pull-Request: https://github.com/git/git/pull/1600
diff.c | 4 ++++
diffcore-delta.c | 14 ++++++++++++++
diffcore-rename.c | 9 +++++++++
diffcore.h | 2 ++
merge-recursive.c | 1 +
5 files changed, 30 insertions(+)
diff --git a/diff.c b/diff.c
index 2c602df10a3..916cc38f57c 100644
--- a/diff.c
+++ b/diff.c
@@ -4081,6 +4081,7 @@ int diff_populate_filespec(struct repository *r,
{
int size_only = options ? options->check_size_only : 0;
int check_binary = options ? options->check_binary : 0;
+ int ignore_whitespace = options ? options->ignore_whitespace : 0;
int err = 0;
int conv_flags = global_conv_flags_eol;
/*
@@ -4090,6 +4091,9 @@ int diff_populate_filespec(struct repository *r,
if (conv_flags & CONV_EOL_RNDTRP_DIE)
conv_flags = CONV_EOL_RNDTRP_WARN;
+ if (ignore_whitespace)
+ s->ignore_whitespace = 1;
+
if (!DIFF_FILE_VALID(s))
die("internal error: asking to populate invalid file.");
if (S_ISDIR(s->mode))
diff --git a/diffcore-delta.c b/diffcore-delta.c
index c30b56e983b..e2bb5f9133a 100644
--- a/diffcore-delta.c
+++ b/diffcore-delta.c
@@ -130,6 +130,7 @@ static struct spanhash_top *hash_chars(struct repository *r,
unsigned char *buf = one->data;
unsigned int sz = one->size;
int is_text = !diff_filespec_is_binary(r, one);
+ int ignore_whitespace = one->ignore_whitespace;
i = INITIAL_HASH_SIZE;
hash = xmalloc(st_add(sizeof(*hash),
@@ -149,6 +150,19 @@ static struct spanhash_top *hash_chars(struct repository *r,
if (is_text && c == '\r' && sz && *buf == '\n')
continue;
+ if (is_text && ignore_whitespace && isspace(c)) {
+ if (sz) {
+ char next = *buf;
+ if ( c == '\n' && next == '\n')
+ continue;
+ else if ( c != '\n' && isspace(next))
+ continue;
+ }
+ if ( c != '\n')
+ /* Normalize whitespace to space*/
+ c = ' ';
+ }
+
accum1 = (accum1 << 7) ^ (accum2 >> 25);
accum2 = (accum2 << 7) ^ (old_1 >> 25);
accum1 += c;
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 5a6e2bcac71..ee68442afc3 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -14,6 +14,7 @@
#include "string-list.h"
#include "strmap.h"
#include "trace2.h"
+#include "xdiff-interface.h"
/* Table of rename/copy destinations */
@@ -950,6 +951,10 @@ static int find_basename_matches(struct diff_options *options,
.info = info
};
+ dpf_options.ignore_whitespace =
+ DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE)
+ || DIFF_XDL_TST(options, IGNORE_WHITESPACE);
+
/*
* Create maps of basename -> fullname(s) for remaining sources and
* dests.
@@ -1402,6 +1407,10 @@ void diffcore_rename_extended(struct diff_options *options,
.repo = options->repo
};
+ dpf_options.ignore_whitespace =
+ DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE)
+ || DIFF_XDL_TST(options, IGNORE_WHITESPACE);
+
trace2_region_enter("diff", "setup", options->repo);
info.setup = 0;
assert(!dir_rename_count || strmap_empty(dir_rename_count));
diff --git a/diffcore.h b/diffcore.h
index 5ffe4ec788f..d6bee67e646 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -61,6 +61,7 @@ struct diff_filespec {
unsigned has_more_entries : 1; /* only appear in combined diff */
/* data should be considered "binary"; -1 means "don't know yet" */
signed int is_binary : 2;
+ unsigned ignore_whitespace : 1;
struct userdiff_driver *driver;
};
@@ -78,6 +79,7 @@ void diff_queued_diff_prefetch(void *repository);
struct diff_populate_filespec_options {
unsigned check_size_only : 1;
unsigned check_binary : 1;
+ unsigned ignore_whitespace : 1;
/*
* If an object is missing, diff_populate_filespec() will invoke this
diff --git a/merge-recursive.c b/merge-recursive.c
index e3beb0801b1..0e52c45158a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1894,6 +1894,7 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *opt,
repo_diff_setup(opt->repo, &opts);
opts.flags.recursive = 1;
opts.flags.rename_empty = 0;
+ opts.xdl_opts = opt->xdl_opts;
opts.detect_rename = merge_detect_rename(opt);
/*
* We do not have logic to handle the detection of copies. In
base-commit: 2e8e77cbac8ac17f94eee2087187fa1718e38b14
--
gitgitgadget
^ permalink raw reply related
* Re: [RFC][Outreachy] Seeking Git Community Feedback on My Application
From: Isoken Ibizugbe @ 2023-10-25 12:45 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <CAP8UFD22EpdBU8HJqFM+=75EBABOTf5a0q+KsbzLK+XTEGSkPw@mail.gmail.com>
On Mon, Oct 23, 2023 at 3:24 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Thu, Oct 19, 2023 at 11:26 AM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
> >
> > Dear Git Community and Mentors,
> >
> > I hope you're doing well. I'm excited to share my application draft
> > for the Outreachy program with the Git project. Your feedback is
> > invaluable, and I'm eager to align the project with the community's
> > needs. Please review the attached draft and share your insights.
>
> Thanks for your project application!
>
> [...]
>
> > Why am I interested in working with the Git chosen project?
> >
> > Git has been a cornerstone for software development, enabling
> > developers worldwide to collaborate, innovate, and create exceptional
> > software. I would say without Git, my journey to pursuing my software
> > engineering career would be impossible, as I use it almost every day.
> > Yet, in this constantly evolving landscape, there is always room for
> > improvement, even in a well-established project. The Git project
> > currently relies on end-to-end tests, and this is where I see an
> > opportunity to make a profound impact. Being able to test libraries in
> > isolation via unit tests or mocks speeds up determining the root cause
> > of bugs. I am deeply passionate about contributing to this project and
> > firmly believe in the power of open-source software and the collective
> > intelligence of the community. A successful completion of this project
> > will significantly improve Git's testing capabilities and bring the
> > benefits of fewer errors, faster work and better testing for all
> > parts.
>
> Ok.
>
> [...]
>
> > Contributions to Git
> >
> > I have actively participated in Git's mailing list discussions and
> > contributed to a micro-project;
> >
> > - builtin/branch.c: Adjust error messages such as die(), error(), and
> > warning() messages used in branch, to conform to coding guidelines
> > (https://lore.kernel.org/git/20231019084052.567922-1-isokenjune@gmail.com/)
> > - Implemented changes to fix broken tests based on reviews from the
> > community (https://lore.kernel.org/git/20231019084052.567922-1-isokenjune@gmail.com/)
> > - In review.
>
> Nice!
>
> > Project Goals:
> >
> > - Improve Testing Efficiency: Transitioning from end-to-end tests to
> > unit tests will enable more efficient testing of error conditions.
> > - Codebase Stability: Unit tests enhance code stability and facilitate
> > easier debugging through isolation.
> > - Simplify Testing: Writing unit tests in pure C simplifies test
> > setup, data passing, and reduces testing runtime by eliminating
> > separate processes for each test.
>
> Ok.
>
> > Project Milestones:
> >
> > - Add useful tests of library-like code
> > - Integrate with stdlib work
>
> Not sure what you call "stdlib" here.
>
> > - Run alongside regular make test target
> >
> > Project Timeline:
> >
> > 1. Oct 2 - Nov 20: Community Bonding
> >
> > - Understanding the structure of Git
> > - Getting familiar with the code
>
> I think some of this time is also spent on working on a microproject,
> writing an application and perhaps doing other things that regular Git
> developers do.
>
> > 2. Dec 4 - Jan 15: Add useful tests of library-like code
> >
> > - Identify and document the current state of the tests in the Git
> > t/helper directory.
>
> It would be nice if you could already take a look at that and tell us
> about it in your application. There are different things in t/helper.
> Some are worth porting and others are not. You might not want (or have
> time to) to classify everything right now, but if you can identify a
> few of each kind, and use those, or just one of them, as an example,
> that would be great.
>
> > - Confirm the licensing and compatibility requirements for the chosen
> > unit testing framework.
>
> I think those who have been working on the unit test framework have
> already done this.
Thank you for the review. I have made changes to the project plan and
it emphasizes the critical tasks of identifying, selecting, and
porting tests, making it more concise and aligned with the project's
scope.
- Community Bonding (Oct 2 - Nov 20): Microproject contribution, Git
project application, get familiar with the Git codebase and testing
ecosystem.
-Identify and Select Tests: Identify and prioritize tests worth
porting, and document the selected tests. (I would classify tests that
are worth porting according to the following for now;
Relevance: Prioritize tests that are relevant to the current Git codebase.
Coverage: Focus on tests that cover core functionality or critical code paths.
Usage Frequency: Port tests that are frequently used or run in Git's
development process.
Isolation: Choose tests that can be easily ported and run independently.
- Write Unit Tests: Write unit tests for the identified test cases
using the Git custom test framework.
- Port Existing Tests: Port selected test cases from the t/helper
directory to the unit testing framework, by modifying them to work
within the custom TAP framework.
- Test Execution and Debugging: Execute the newly written unit tests
and the ported tests using the test framework.
- Seek Feedback: Share the progress with mentors and the Git
community, and address any concerns or suggestions provided by the
community.
- Documentation and Reporting: Document the entire process of
migrating Git's tests to the unit testing framework, and prepare a
final project report summarizing the work done, challenges faced, and
lessons learned.
What is the custom TAP framework?
According to this patch
(https://lore.kernel.org/git/ca284c575ece0aee7149641d5fb1977ccd7e7873.1692229626.git.steadmon@google.com/)
by Phillip Wood, which contains an example implementation for writing
unit tests with TAP output. The custom TAP framework is a Test
Anything Protocol (TAP) framework that allows for clear reporting of
test results, aiding in debugging and troubleshooting.
The framework contains the following features:
- Test Structure: Unit tests are defined as functions containing
multiple checks. The tests are run using the TEST() macro. If any
checks within a test fail, the entire test is marked as failed.
- Output Format: The output of the test program follows the TAP
format. It includes a series of messages describing the test's status.
For passed tests, it reports "ok," and for failed tests, it reports
"not ok." Each test is numbered, e.g., "ok 1 - static initialization
works," to indicate success or failure.
- Check Functions: Several check functions are available, including
check() for boolean conditions, check_int(), check_uint(), and
check_char() for comparing values using various operators. check_str()
is used to compare strings.
- Skipping Tests: Tests can be skipped using test_skip() and can
include a reason for skipping, which is printed as part of the report.
- Diagnostic Messages: Tests can generate diagnostic messages using
test_msg() to provide additional context or explanations for test
failures.
- Planned Failing Tests: Tests that are known to fail can be marked
with TEST_TODO(). These tests will still run, and the failures will be
reported, but they will not cause the entire suite to fail.
- Building and Running: The unit tests can be built with "make
unit-tests" (with some additional Makefile changes), and they can be
executed manually or using a tool like prove.
Using the formerly given criteria, test-ctype.c is suitable for
porting because it tests character type checks used extensively in
Git. These tests cover various character types and their expected
behaviour, ensuring the correctness and reliability of Git's
operations, and test-ctype.c isolation makes it suitable for porting
without relying on multiple libraries.
Here is a sample of the implementation of how I would write the unit
test following the custom TAP framework taking t/helper/test-ctype.c
- Create and rename the new .c file;
I would rename it according to the convention done in the t/unit-test
directory, by starting the name with a “t-” prefix e.g t-ctype.c
- Document the tests and include the necessary headers:
/**
*Tests the behavior of ctype
*functions
*/
#include "test-lib.h"
#include "ctype.h"
- Define test functions:
#define DIGIT "0123456789"
static void t_digit_type(void)
{
int i;
const char *digits = DIGIT;
for (i = 0; digits[i]; i++)
{
check_int(isdigit(digits[i]), ==, 0);
}
- Include main function which will call the test functions using the TEST macro;
int main(void)
{
TEST(t_digit_type(), "Character is a digit");
return test_done();
}
- Run the tests:
‘make && make’ unit-tests can be used build and run the unit tests
Or run the test binaries directly:
./t/unit-tests/t-ctype.c
The Makefile will be modified to add the file;
UNIT_TEST_PROGRAMS += t-ctype
The test output will be in the TAP format and will indicate which
tests passed(ok) and which failed(not ok), along with diagnostic
messages in case of failures.
ok 1 - Character is a digit
1..1
>
> > - Develop unit tests for these library-like components.
>
> Not sure what are "these library-like components". An example would
> perhaps help.
>
> > - Execute the tests and ensure they cover various scenarios, including
> > error conditions.
> > - Run the tests and address any initial issues or bugs to ensure they
> > work as intended.
>
> Ok.
>
> > - Document the new tests and their coverage.
>
> What kind of documentation would that be?
>
> > - Seek feedback and support from mentors and the Git community
> >
> > 3. Jan 15 - Feb 15: Integrate with Stdlib Work
> >
> > - Collaborate with the team working on standard library integration.
>
> Not sure what "standard library". Actually, maybe you are talking
> about the goal of having a "standard library" implementation for Git
> which is described in this report from the Virtual Contributor's
> Summit:
>
> https://lore.kernel.org/git/ZRrfN2lbg14IOLiK@nand.local/
>
> It's true that the unit test framework would help with that goal. So
> yeah maybe you will have to collaborate with the team working on that
> goal. I am not sure at what step the work on this library will be when
> the internship will start though.
>
> > - Ensure that the tests for library-like code align with stdlib work.
> > - Verify that the tests effectively check the compatibility and
> > interaction of the code with standard libraries.
> > - Gather feedback and insights from the Git community on the
> > integrated tests, addressing any concerns or suggestions.
>
> Ok, but I think it would be more interesting to follow the steps with
> an example test.
>
> > 4. Feb 15 - March 1: Run Alongside Regular 'make test' Target and finalize
> >
> > - Configure the testing framework to run alongside the regular 'make
> > test' target.
>
> I think others will likely take care of that sooner.
>
> > - Ensure that the new tests are included in the standard testing suite.
> > - Execute 'make test' with the new tests and verify that they pass successfully.
> > - Document the integration process and how the new tests are included
> > in the standard testing procedure.
> > - Perform comprehensive testing of the entire unit testing framework.
> > - Ensure all migrated tests are working correctly within the new framework.
> > - Document the entire process of migrating Git's tests
> > - Prepare a final project report
>
> Ok, but here also following an example test would be more interesting.
>
> > Technical Requirements
> >
> > According to the documentation on the unit test project
> > (https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc),
> > the suggested best framework for the Git project is the "Custom TAP
> > framework" (Phillip Wood's TAP implementation), as it aligns with
> > Git's licensing requirements, is vendorable, and can be customized by
> > Git's developers as needed, but it may require some additional
> > development work for features like parallel execution and mock
> > support, but it offers a strong foundation for unit testing within the
> > Git project.
>
> Yeah, right. Thanks for summarizing that document!
>
> > Relevant Projects
> >
> > Simple shell - A project based on emulating a shell. It was a
> > collaborative project which we managed using Git.
> > (https://github.com/Junie06/simple_shell).
> > This project was written in C, which allowed me to apply my C language
> > knowledge, essential for Git projects.
> > I'm proficient in using Shell for scripting, redirections, and
> > permissions, as shown in my work
> > (https://github.com/Junie06/alx-system_engineering-devops).
> > Creating the simple shell project deepened my understanding of how
> > shells work, and I even attempted to replicate a shell environment.
> > Collaborating on the Simple Shell project reinforced my Git skills.
>
> Ok, nice!
>
> Best,
> Christian.
^ permalink raw reply
* Re: [PATCH 00/12] show-ref: introduce mode to check for ref existence
From: Han-Wen Nienhuys @ 2023-10-25 14:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git, Eric Sunshine
In-Reply-To: <xmqqttqf3k5a.fsf@gitster.g>
On Tue, Oct 24, 2023 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > this patch series introduces a new `--exists` mode to git-show-ref(1) to
> > explicitly check for the existence of a reference, only.
>
> I agree that show-ref would be the best place for this feature (not
> rev-parse, which is already a kitchen sink). After all, the command
> was designed for validating refs in 358ddb62 (Add "git show-ref"
> builtin command, 2006-09-15).
>
> Thanks. Hopefully I can take a look before I go offline.
The series description doesn't say why users would care about this.
If this is just to ease testing, I suggest adding functionality to a
suitable test helper. Anything you add to git-show-ref is a publicly
visible API that needs documentation and comes with a stability
guarantee that is more expensive to maintain than test helper
functionality.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Liana Sebastian
^ permalink raw reply
* Re: [PATCH 00/12] show-ref: introduce mode to check for ref existence
From: Phillip Wood @ 2023-10-25 14:44 UTC (permalink / raw)
To: Han-Wen Nienhuys, Junio C Hamano; +Cc: Patrick Steinhardt, git, Eric Sunshine
In-Reply-To: <CAFQ2z_PqNsz+zycSxz=q2cUVOpJS-AEjwHxEM-fiafxd3dxc9g@mail.gmail.com>
On 25/10/2023 15:26, Han-Wen Nienhuys wrote:
> On Tue, Oct 24, 2023 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>>> this patch series introduces a new `--exists` mode to git-show-ref(1) to
>>> explicitly check for the existence of a reference, only.
>>
>> I agree that show-ref would be the best place for this feature (not
>> rev-parse, which is already a kitchen sink). After all, the command
>> was designed for validating refs in 358ddb62 (Add "git show-ref"
>> builtin command, 2006-09-15).
>>
>> Thanks. Hopefully I can take a look before I go offline.
>
> The series description doesn't say why users would care about this.
>
> If this is just to ease testing, I suggest adding functionality to a
> suitable test helper. Anything you add to git-show-ref is a publicly
> visible API that needs documentation and comes with a stability
> guarantee that is more expensive to maintain than test helper
> functionality.
Does the new functionality provide a way for scripts to see if a branch
is unborn (i.e. has not commits yet)? I don't think we have a way to
distinguish between a ref that points to a missing object and an unborn
branch at the moment.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v5 1/5] bulk-checkin: extract abstract `bulk_checkin_source`
From: Taylor Blau @ 2023-10-25 15:39 UTC (permalink / raw)
To: Jeff King
Cc: git, Elijah Newren, Eric W. Biederman, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <20231025073736.GB2145145@coredump.intra.peff.net>
On Wed, Oct 25, 2023 at 03:37:36AM -0400, Jeff King wrote:
> I don't mind this in-between state. It is a funny layering violating
> from an OO standpoint, but it's not like we expect an unbounded set of
> concrete types to "inherit" from the source struct.
Yeah, this was exactly my thinking when writing up the changes for this
round. Since all of the "sub-classes" are local to the bulk-checkin.o
compilation unit, I don't have grave concerns about one implementation
peering into the details of another's.
Gotta stop somewhere ;-).
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-25 15:44 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <ZTjKjkRMkmCuxDU1@tanuki>
On Wed, Oct 25, 2023 at 09:58:06AM +0200, Patrick Steinhardt wrote:
> On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:
> > Introduce `index_blob_bulk_checkin_incore()` which allows streaming
> > arbitrary blob contents from memory into the bulk-checkin pack.
> >
> > In order to support streaming from a location in memory, we must
> > implement a new kind of bulk_checkin_source that does just that. These
> > implementation in spread out across:
>
> Nit: the commit message is a bit off here. Probably not worth a reroll
> though.
Your eyes are definitely mine, because I'm not seeing where the commit
message is off! But hopefully since you already don't think it's worth a
reroll, and I'm not even sure what the issue is that we can just leave
it ;-).
> > +static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,
> > + off_t offset)
> > +{
> > + if (!(0 <= offset && offset < source->size))
> > + return (off_t)-1;
>
> At the risk of showing my own ignorance, but why is the cast here
> necessary?
It's not necessary, see this godbolt example that shows two functions
doing the same thing (one with the explicit cast, one without):
https://godbolt.org/z/f737EcGfG
But there are a couple of other spots within the bulk-checkin code
(specifically within the deflate_blob_to_pack() routine) that explicitly
cast -1 to an off_t, so I was more trying to imitate the local style.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Taylor Blau @ 2023-10-25 15:46 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <ZTjKk8E55M7lQN1m@tanuki>
On Wed, Oct 25, 2023 at 09:58:11AM +0200, Patrick Steinhardt wrote:
> > +test_expect_success 'merge-tree can pack its result with --write-pack' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > +
> > + # base has lines [3, 4, 5]
> > + # - side adds to the beginning, resulting in [1, 2, 3, 4, 5]
> > + # - other adds to the end, resulting in [3, 4, 5, 6, 7]
> > + #
> > + # merging the two should result in a new blob object containing
> > + # [1, 2, 3, 4, 5, 6, 7], along with a new tree.
> > + test_commit -C repo base file "$(test_seq 3 5)" &&
> > + git -C repo branch -M main &&
> > + git -C repo checkout -b side main &&
> > + test_commit -C repo side file "$(test_seq 1 5)" &&
> > + git -C repo checkout -b other main &&
> > + test_commit -C repo other file "$(test_seq 3 7)" &&
> > +
> > + find repo/$packdir -type f -name "pack-*.idx" >packs.before &&
> > + tree="$(git -C repo merge-tree --write-pack \
> > + refs/tags/side refs/tags/other)" &&
> > + blob="$(git -C repo rev-parse $tree:file)" &&
> > + find repo/$packdir -type f -name "pack-*.idx" >packs.after &&
>
> While we do assert that we write a new packfile, we don't assert whether
> parts of the written object may have been written as loose objects. Do
> we want to tighten the checks to verify that?
We could, but the tests in t1050 already verify this, so I'm not sure
that we would be significantly hardening our test coverage here. If you
feel strongly about it, though, I'm happy to change it up.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Kristoffer Haugsbakk @ 2023-10-25 16:27 UTC (permalink / raw)
To: Ruslan Yakauleu; +Cc: git, Josh Soref
In-Reply-To: <pull.1599.git.git.1698224280816.gitgitgadget@gmail.com>
Hi
On Wed, Oct 25, 2023, at 10:58, Ruslan Yakauleu via GitGitGadget wrote:
> A new option --ff-one-only to control the merging strategy.
> For one commit option works like -ff to avoid extra merge commit.
> In other cases the option works like --no-ff to create merge commit for
> complex features.
I do something similar for my pull requests:
- If more than one commit: `--no-ff`
- One commit: rebase/squash
Your change would (according to the commit message) only *not* create a
merge if it so happens to be the case that you can fast-forward the
branch. So it would create a merge commit if (say) your branch was three
commits behind and one ahead (your commit) of the target branch. But isn’t
it fine to just rebase/squash in this case? That seems more general.
(Maybe `--squash-one-only`.)
But it seems that your new option would work nicely with “semi-linear”
merges:[1]
- Rebase on the target branch
- `--ff-one-only`
- Now you either get:
- a “semi-linear merge”; or
- a single commit (as opposed to a merge which joins together a branch
which is 0 behind and 1 ahead of the target branch)
--
Kristoffer
^ permalink raw reply
* Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Eric Sunshine @ 2023-10-25 17:21 UTC (permalink / raw)
To: Taylor Blau
Cc: Patrick Steinhardt, git, Elijah Newren, Eric W. Biederman,
Jeff King, Junio C Hamano
In-Reply-To: <ZTk3zoncT6nvV3aQ@nand.local>
On Wed, Oct 25, 2023 at 11:44 AM Taylor Blau <me@ttaylorr.com> wrote:
> On Wed, Oct 25, 2023 at 09:58:06AM +0200, Patrick Steinhardt wrote:
> > On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:
> > > In order to support streaming from a location in memory, we must
> > > implement a new kind of bulk_checkin_source that does just that. These
> > > implementation in spread out across:
> >
> > Nit: the commit message is a bit off here. Probably not worth a reroll
> > though.
>
> Your eyes are definitely mine, because I'm not seeing where the commit
> message is off! But hopefully since you already don't think it's worth a
> reroll, and I'm not even sure what the issue is that we can just leave
> it ;-).
Perhaps:
s/implementation in/implementations are/
^ permalink raw reply
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Ruslan Yakauleu @ 2023-10-25 18:31 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <da246287-8530-4680-8fcc-f68f881bc24b@app.fastmail.com>
Hi
If we use squash we will lose some context and occasionally, we need
multiple small features combined into one big release. We would rather
not mix it into one monolithic non-readable blob. For us, sometimes it
is better to rebase something to make history more accurate than squash
everything into one commit. We can use squash only for one story.
Anyway, squash is a different feature.
Same as rebase (of course we're doing the rebase before merge to clarify
history and to make some regression tests)
Then we have a set of branches. Some branches contain only one commit.
For example, we have 4 branches:
- two commits
- one commit
- one commit
- three commits
With --no-ff (only merges) we have next graph with extra merges (E', F')
for branches with one commit
B---C E F G---H---I
/ \ / \ / \ / \
A-------D---E'--F'----------J
With --ff (fast-forward everything) we miss merge branches (D, J) and
it's harder to fast revert some problematic releases properly, because
it's not clear that commits G-H-I - is one release
A---B---C---E---F---G---H---I
Story which --ff-one-only should build without manual control
B---C G---H---I
/ \ / \
A-------D---E---F-----------J
There we have merge commits (D, J) only for complex branches.
Branches E and F fast-forwarded to prevent extra merges.
Sorry if my explanation isn't clear enough
--
Ruslan
^ permalink raw reply
* Re: [PATCH] send-email: move validation code below process_address_list
From: Michael Strawbridge @ 2023-10-25 18:47 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
In-Reply-To: <20231025065033.GB2094463@coredump.intra.peff.net>
On 10/25/23 02:50, Jeff King wrote:
> On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
>
>> Move validation logic below processing of email address lists so that
>> email validation gets the proper email addresses.
>>
>> This fixes email address validation errors when the optional
>> perl module Email::Valid is installed and multiple addresses are passed
>> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
>
> Is there a test we can include here?
>
>> @@ -2023,6 +1999,30 @@ sub process_file {
>> return 1;
>> }
>>
>> +if ($validate) {
>
> So the new spot is right before we call process_file() on each of the
> input files. It is a little hard to follow because of the number of
> functions defined inline in the middle of the script, but I think that
> is a reasonable spot. It is after we have called process_address_list()
> on to/cc/bcc, which I think fixes the regression. But it is also after
> we sanitize $reply_to, etc, which seems like a good idea.
>
> But I think putting it down that far is the source of the test failures.
>
> The culprit seems not to be the call to validate_patch() in the loop you
> moved, but rather pre_process_file(), which was added in your earlier
> a8022c5f7b (send-email: expose header information to git-send-email's
> sendemail-validate hook, 2023-04-19).
>
> It looks like the issue is the global $message_num variable which is
> incremented by pre_process_file(). On line 1755 (on the current tip of
> master), we set it to 0. And your patch moves the validation across
> there (from line ~799 to ~2023).
>
> And that's why the extra increments didn't matter when you added the
> calls to pre_process_file() in your earlier patch; they all happened
> before we reset $message_num to 0. But now they happen after.
>
> To be fair, this is absolutely horrific perl code. There's over a
> thousand lines of function definitions, and then hidden in the middle
> are some global variable assignments!
I agree. Following where things are initialized seems to be especially troublesome.
>
> So we have a few options, I think:
>
> 1. Reset $message_num to 0 after validating (maybe we also need
> to reset $in_reply_to, etc, set at the same time? I didn't check).
> This feels like a hack.
>
> 2. Move the validation down, but not so far down. Like maybe right
> after we set up the @files list with the $compose.final name. This
> is the smallest diff, but feels like we haven't really made the
> world a better place.
>
> 3. Move the $message_num, etc, initialization to right before we call
> the process_file() loop, which is what expects to use them. Like
> this (squashed into your patch):
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index a898dbc76e..d44db14223 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1730,10 +1730,6 @@ sub send_message {
> return 1;
> }
>
> -$in_reply_to = $initial_in_reply_to;
> -$references = $initial_in_reply_to || '';
> -$message_num = 0;
> -
> sub pre_process_file {
> my ($t, $quiet) = @_;
>
> @@ -2023,6 +2019,10 @@ sub process_file {
> delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
> }
>
> +$in_reply_to = $initial_in_reply_to;
> +$references = $initial_in_reply_to || '';
> +$message_num = 0;
> +
> foreach my $t (@files) {
> while (!process_file($t)) {
> # user edited the file
>
The above patch was a great place to start. Thank you! In order to address
the fact that validation and actually sending the emails should have the same
initial conditions I created a new function to set the variables and call it
instead.
> That seems to make the test failures go away. It is still weird that the
> validation code is calling pre_process_file(), which increments
> $message_num, without us having set it up in any meaningful way. I'm not
> sure if there are bugs lurking there or not. I'm not impressed by the
> general quality of this code, and I'm kind of afraid to keep looking
> deeper.
>
> -Peff
^ permalink raw reply
* Re: [PATCH] send-email: move validation code below process_address_list
From: Michael Strawbridge @ 2023-10-25 18:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
In-Reply-To: <xmqqil6v3cgq.fsf@gitster.g>
On 10/24/23 18:03, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Michael Strawbridge <michael.strawbridge@amd.com> writes:
>>
>>> Subject: [PATCH] send-email: move validation code below process_address_list
>>>
>>> Move validation logic below processing of email address lists so that
>>> email validation gets the proper email addresses.
>>
>> Hmph, without this patch, the tip of 'seen' passes t9001 on my box,
>> but with it, it claims that it failed 58, 87, and 89.
>
> FWIW, when this patch is used with 'master' (not 'seen'), t9001
> claims the same three tests failed. The way #58 fails seems to be
> identical to the way 'seen' with this patch failed, shown in the
> message I am responding to.
I'm sorry to have wasted your time with patch 1. I had done the other manual
tests but ended up forgetting the automated ones.
^ permalink raw reply
* [PATCH v2] send-email: move validation code below process_address_list
From: Michael Strawbridge @ 2023-10-25 18:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
In-Reply-To: <xmqqil6v3cgq.fsf@gitster.g>
From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
From: Michael Strawbridge <michael.strawbridge@amd.com>
Date: Wed, 11 Oct 2023 16:13:13 -0400
Subject: [PATCH v2] send-email: move validation code below
process_address_list
Move validation logic below processing of email address lists so that
email validation gets the proper email addresses. As a side effect,
some initialization needed to be moved down. In order for validation
and the actual email sending to have the same initial state, the
initialized variables that get modified by pre_process_file are
encapsulated in a new function.
This fixes email address validation errors when the optional
perl module Email::Valid is installed and multiple addresses are passed
in on a single to/cc argument like --to=foo@example.com,bar@example.com.
A new test was added to t9001 to expose failures with this case in the
future.
Fixes: a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
git-send-email.perl | 60 +++++++++++++++++++++++--------------------
t/t9001-send-email.sh | 19 ++++++++++++++
2 files changed, 51 insertions(+), 28 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..ce22a5e06d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -799,30 +799,6 @@ sub is_format_patch_arg {
$time = time - scalar $#files;
-if ($validate) {
- # FIFOs can only be read once, exclude them from validation.
- my @real_files = ();
- foreach my $f (@files) {
- unless (-p $f) {
- push(@real_files, $f);
- }
- }
-
- # Run the loop once again to avoid gaps in the counter due to FIFO
- # arguments provided by the user.
- my $num = 1;
- my $num_files = scalar @real_files;
- $ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
- foreach my $r (@real_files) {
- $ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
- pre_process_file($r, 1);
- validate_patch($r, $target_xfer_encoding);
- $num += 1;
- }
- delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
- delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
-}
-
@files = handle_backup_files(@files);
if (@files) {
@@ -1754,10 +1730,6 @@ sub send_message {
return 1;
}
-$in_reply_to = $initial_in_reply_to;
-$references = $initial_in_reply_to || '';
-$message_num = 0;
-
sub pre_process_file {
my ($t, $quiet) = @_;
@@ -2023,6 +1995,38 @@ sub process_file {
return 1;
}
+sub initialize_modified_loop_vars {
+ $in_reply_to = $initial_in_reply_to;
+ $references = $initial_in_reply_to || '';
+ $message_num = 0;
+}
+
+if ($validate) {
+ # FIFOs can only be read once, exclude them from validation.
+ my @real_files = ();
+ foreach my $f (@files) {
+ unless (-p $f) {
+ push(@real_files, $f);
+ }
+ }
+
+ # Run the loop once again to avoid gaps in the counter due to FIFO
+ # arguments provided by the user.
+ my $num = 1;
+ my $num_files = scalar @real_files;
+ $ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
+ initialize_modified_loop_vars();
+ foreach my $r (@real_files) {
+ $ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
+ pre_process_file($r, 1);
+ validate_patch($r, $target_xfer_encoding);
+ $num += 1;
+ }
+ delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
+ delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
+}
+
+initialize_modified_loop_vars();
foreach my $t (@files) {
while (!process_file($t)) {
# user edited the file
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 263db3ad17..ccff2ad647 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -633,6 +633,25 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
test_cmp expect actual
'
+test_expect_success $PREREQ "--validate hook supports multiple addresses in arguments" '
+ hooks_path="$(pwd)/my-hooks" &&
+ test_config core.hooksPath "$hooks_path" &&
+ test_when_finished "rm my-hooks.ran" &&
+ test_must_fail git send-email \
+ --from="Example <nobody@example.com>" \
+ --to=nobody@example.com,abc@example.com \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ --validate \
+ longline.patch 2>actual &&
+ test_path_is_file my-hooks.ran &&
+ cat >expect <<-EOF &&
+ fatal: longline.patch: rejected by sendemail-validate hook
+ fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
+ warning: no patches were sent
+ EOF
+ test_cmp expect actual
+'
+
test_expect_success $PREREQ "--validate hook supports header argument" '
write_script my-hooks/sendemail-validate <<-\EOF &&
if test "$#" -ge 2
--
2.42.GIT
^ permalink raw reply related
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Kristoffer Haugsbakk @ 2023-10-25 19:04 UTC (permalink / raw)
To: Ruslan Yakauleu; +Cc: git
In-Reply-To: <e38ebf04-cf92-c80b-3432-bf86ecda1054@gmail.com>
On Wed, Oct 25, 2023, at 20:31, Ruslan Yakauleu wrote:
> Hi
>
> If we use squash we will lose some context and occasionally, we need
> multiple small features combined into one big release. We would rather
> not mix it into one monolithic non-readable blob.
The only context you lose is the point where your branch started, if that
matters. (Later in the email you say that you rebase before you merge so
apparently it does not matter to you.)
To squash one single commit doesn’t make a “monolithic” commit—it is
exactly as monolithic as the commit you started with.
> For us, sometimes it is better to rebase something to make history more
> accurate than squash everything into one commit. We can use squash only
> for one story.
No, squash one single commit. Not multiple.
> Anyway, squash is a different feature. Same as rebase (of course we're
> doing the rebase before merge to clarify history and to make some
> regression tests)
Squash and rebase are functionally identical in this case. And since you
rebase before merge anyway it is identical to the flow of
1. Rebase before merge
2. Merge with your new option
Except you don’t have to split it into two steps.
^ permalink raw reply
* Re: [PATCH v3 0/5] config-parse: create config parsing library
From: Josh Steadmon @ 2023-10-25 19:37 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Taylor Blau, Junio C Hamano, git, calvinwan, glencbz
In-Reply-To: <20231024225005.1191555-1-jonathantanmy@google.com>
On 2023.10.24 15:50, Jonathan Tan wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> > But I am not sure that I agree that this series is moving us in the
> > right direction necessarily. Or at least I am not convinced that
> > shipping the intermediate state is worth doing before we have callers
> > that could drop '#include "config.h"' for just the parser.
> >
> > This feels like churn that does not yield a tangible pay-off, at least
> > in the sense that the refactoring and code movement delivers us
> > something that we can substantively use today.
> >
> > I dunno.
> >
> > Thanks,
> > Taylor
>
> Thanks for calling this out. We do want our changes to be good for both
> the libification and the non-libification cases as much as possible. As
> it is, I do agree that since we won't have callers that can use the new
> parser header (I think the likeliest cause of having such a caller is
> if we have a "interpret-config" command, like "interpret-trailers"), we
> probably shouldn't merge this (at least, the last 2 patches).
>
> I think patches 1-3 are still usable (they make some internals of config
> parsing less confusing) but I'm also OK if we hold off on them until
> we find a compelling use case that motivates refactoring on the config
> parser.
Thanks everyone for the revived discussion here. I think I agree, this
series is not going in the right direction. Additionally, our internal
use case for this change has evaporated, so let's just drop the series.
We can pick it up again later if interest returns.
^ 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