* Re: [PATCH] send-email: move validation code below process_address_list
From: Jeff King @ 2023-10-25 6:50 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
In-Reply-To: <ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com>
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!
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
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 related
* Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option
From: Patrick Steinhardt @ 2023-10-25 6:40 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, gitster
In-Reply-To: <20231024122631.158415-4-karthik.188@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8550 bytes --]
On Tue, Oct 24, 2023 at 02:26:31PM +0200, Karthik Nayak wrote:
> The `--missing` object option in rev-list currently works only with
> missing blobs/trees. For missing commits the revision walker fails with
> a fatal error.
>
> Let's extend the functionality of `--missing` option to also support
> commit objects. This is done by adding a `missing_objects` field to
> `rev_info`. This field is an `oidset` to which we'll add the missing
> commits as we encounter them. The revision walker will now continue the
> traversal and call `show_commit()` even for missing commits. In rev-list
> we can then check if the commit is a missing commit and call the
> existing code for parsing `--missing` objects.
>
> A scenario where this option would be used is to find the boundary
> objects between different object directories. Consider a repository with
> a main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate
> object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a
> repository, using the `--missing=print` option while disabling the
> alternate object directory allows us to find the boundary objects
> between the main and alternate object directory.
>
> Helped-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> builtin/rev-list.c | 6 +++
> list-objects.c | 3 ++
> revision.c | 16 +++++++-
> revision.h | 4 ++
> t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 101 insertions(+), 2 deletions(-)
> create mode 100755 t/t6022-rev-list-missing.sh
>
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 98542e8b3c..37b52520b5 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)
>
> display_progress(progress, ++progress_counter);
>
> + if (revs->do_not_die_on_missing_objects &&
> + oidset_contains(&revs->missing_objects, &commit->object.oid)) {
> + finish_object__ma(&commit->object);
> + return;
> + }
> +
> if (show_disk_usage)
> total_disk_usage += get_object_disk_usage(&commit->object);
>
> diff --git a/list-objects.c b/list-objects.c
> index 47296dff2f..260089388c 100644
> --- a/list-objects.c
> +++ b/list-objects.c
> @@ -389,6 +389,9 @@ static void do_traverse(struct traversal_context *ctx)
> */
> if (!ctx->revs->tree_objects)
> ; /* do not bother loading tree */
> + else if (ctx->revs->do_not_die_on_missing_objects &&
> + oidset_contains(&ctx->revs->missing_objects, &commit->object.oid))
> + ;
> else if (repo_get_commit_tree(the_repository, commit)) {
> struct tree *tree = repo_get_commit_tree(the_repository,
> commit);
> diff --git a/revision.c b/revision.c
> index 219dc76716..e60646c1a7 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -6,6 +6,7 @@
> #include "object-name.h"
> #include "object-file.h"
> #include "object-store-ll.h"
> +#include "oidset.h"
> #include "tag.h"
> #include "blob.h"
> #include "tree.h"
> @@ -1112,6 +1113,9 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
>
> 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.
> + return 0;
> commit->object.flags |= ADDED;
>
> if (revs->include_check &&
> @@ -1168,7 +1172,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
> for (parent = commit->parents; parent; parent = parent->next) {
> struct commit *p = parent->item;
> int gently = revs->ignore_missing_links ||
> - revs->exclude_promisor_objects;
> + revs->exclude_promisor_objects ||
> + revs->do_not_die_on_missing_objects;
> if (repo_parse_commit_gently(revs->repo, p, gently) < 0) {
> if (revs->exclude_promisor_objects &&
> is_promisor_object(&p->object.oid)) {
> @@ -1176,7 +1181,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
> break;
> continue;
> }
> - return -1;
> +
> + if (!revs->do_not_die_on_missing_objects)
> + return -1;
> + else
> + oidset_insert(&revs->missing_objects, &p->object.oid);
> }
> if (revs->sources) {
> char **slot = revision_sources_at(revs->sources, p);
> @@ -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);
> +
While we're initializing the new oidset, we never clear it. We should
probably call `oidset_clear()` in `release_revisions()`. And if we
unconditionally initialized the oidset here then we can also call
`oiadset_clear()` unconditionally there, which should be preferable
given that `oidset_init()` does not allocate memory when no initial size
was given.
> if (!revs->reflog_info)
> prepare_to_use_bloom_filter(revs);
> if (!revs->unsorted_input)
> diff --git a/revision.h b/revision.h
> index c73c92ef40..f6bf422f0e 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -4,6 +4,7 @@
> #include "commit.h"
> #include "grep.h"
> #include "notes.h"
> +#include "oidset.h"
> #include "pretty.h"
> #include "diff.h"
> #include "commit-slab-decl.h"
> @@ -373,6 +374,9 @@ struct rev_info {
>
> /* Location where temporary objects for remerge-diff are written. */
> struct tmp_objdir *remerge_objdir;
> +
> + /* 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?
Patrick
> };
>
> /**
> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
> new file mode 100755
> index 0000000000..40265a4f66
> --- /dev/null
> +++ b/t/t6022-rev-list-missing.sh
> @@ -0,0 +1,74 @@
> +#!/bin/sh
> +
> +test_description='handling of missing objects in rev-list'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
> +. ./test-lib.sh
> +
> +# We setup the repository with two commits, this way HEAD is always
> +# available and we can hide commit 1.
> +test_expect_success 'create repository and alternate directory' '
> + test_commit 1 &&
> + test_commit 2 &&
> + test_commit 3
> +'
> +
> +for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
> +do
> + test_expect_success "rev-list --missing=error fails with missing object $obj" '
> + oid="$(git rev-parse $obj)" &&
> + path=".git/objects/$(test_oid_to_path $oid)" &&
> +
> + mv "$path" "$path.hidden" &&
> + test_when_finished "mv $path.hidden $path" &&
> +
> + test_must_fail git rev-list --missing=error --objects \
> + --no-object-names HEAD
> + '
> +done
> +
> +for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
> +do
> + for action in "allow-any" "print"
> + do
> + test_expect_success "rev-list --missing=$action with missing $obj" '
> + oid="$(git rev-parse $obj)" &&
> + path=".git/objects/$(test_oid_to_path $oid)" &&
> +
> + # Before the object is made missing, we use rev-list to
> + # get the expected oids.
> + git rev-list --objects --no-object-names \
> + HEAD ^$obj >expect.raw &&
> +
> + # Blobs are shared by all commits, so evethough a commit/tree
> + # might be skipped, its blob must be accounted for.
> + if [ $obj != "HEAD:1.t" ]; then
> + echo $(git rev-parse HEAD:1.t) >>expect.raw &&
> + echo $(git rev-parse HEAD:2.t) >>expect.raw
> + fi &&
> +
> + mv "$path" "$path.hidden" &&
> + test_when_finished "mv $path.hidden $path" &&
> +
> + git rev-list --missing=$action --objects --no-object-names \
> + HEAD >actual.raw &&
> +
> + # When the action is to print, we should also add the missing
> + # oid to the expect list.
> + case $action in
> + allow-any)
> + ;;
> + print)
> + grep ?$oid actual.raw &&
> + echo ?$oid >>expect.raw
> + ;;
> + esac &&
> +
> + sort actual.raw >actual &&
> + sort expect.raw >expect &&
> + test_cmp expect actual
> + '
> + done
> +done
> +
> +test_done
> --
> 2.42.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Jeff King @ 2023-10-25 6:11 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
In-Reply-To: <ZTbOnsxBFERPLN3F@ugly>
On Mon, Oct 23, 2023 at 09:50:54PM +0200, Oswald Buddenhagen wrote:
> > The "//" one would
> > work, but we support perl versions old enough that they don't have it.
> >
> according to my grepping, that ship has sailed.
> also, why _would_ you support such ancient perl versions? that makes even
> less sense to me than supporting ancient c compilers.
It may be reasonable to bump the default perl version for the script.
But that would require somebody digging into what tends to ship these
days (which can be sometimes be surprising; witness macos using old
versions of bash due to license issues), and then updating the "use
5.008" in the script.
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). But
if we are not consistent with the "use" claim, that is worth fixing.
-Peff
^ permalink raw reply
* Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
From: Patrick Steinhardt @ 2023-10-25 5:38 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Taylor Blau, git, stolee
In-Reply-To: <87badbe0-de18-4f8a-9589-314cea46065e@app.fastmail.com>
[-- Attachment #1: Type: text/plain, Size: 2034 bytes --]
On Tue, Oct 24, 2023 at 03:23:11PM +0200, Kristoffer Haugsbakk wrote:
> Hi Taylor and Patrick
>
> On Mon, Oct 23, 2023, at 19:40, Taylor Blau wrote:
> >> Nit: we don't know about the intent of the caller, so they may not want
> >> to write to the file but only read it.
> >
> > I was going to suggest that we allow the caller to pass in the flags
> > that they wish for git_global_config() to pass down to access(2), but
> > was surprised to see that we always use R_OK.
> >
> > But thinking on it for a moment longer, I realized that we don't care
> > about write-level permissions for the config, since we want to instead
> > open $GIT_DIR/config.lock for writing, and then rename() it into place,
> > meaning we only care about whether or not we have write permissions on
> > $GIT_DIR itself.
> >
> > I think in the existing location of this code, the "if we should write"
> > portion of the comment is premature, since we don't know for sure
> > whether or not we are writing. So I'd be fine with leaving it as-is, but
> > changing the comment seems easy enough to do...
> >
> >> > + * location; error out even if XDG_CONFIG_HOME
> >> > + * is set and points at a sane location.
> >> > + */
> >> > + die(_("$HOME not set"));
> >>
> >> Is it sensible to `die()` here in this new function that behaves more
> >> like a library function? I imagine it would be more sensible to indicate
> >> the error to the user and let them handle it accordingly.
> >
> > Agreed.
> >
> > Thanks,
> > Taylor
>
> What do you guys think the signature of `git_global_config` should be?
Either of the following:
- `int git_global_config(char **out_pat)`
- `char **git_global_config(void)`
In the first case you'd signal error via a non-zero return value,
whereas in the second case you would signal it via a `NULL` return
value.
To decide which one to go with I'd recommend to check whether there is
any similar precedent in "config.h" and what style that precedent uses.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option
From: Junio C Hamano @ 2023-10-25 0:35 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, ps
In-Reply-To: <xmqqbkcn52z4.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> 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)
^ permalink raw reply related
* Re: [PATCH v3 0/5] config-parse: create config parsing library
From: Jonathan Tan @ 2023-10-24 22:50 UTC (permalink / raw)
To: Taylor Blau
Cc: Jonathan Tan, Junio C Hamano, Josh Steadmon, git, calvinwan,
glencbz
In-Reply-To: <ZTbK3QTJYXxYj/M6@nand.local>
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.
^ permalink raw reply
* Re: [PATCH] SubmittingPatches: call gitk's command "Copy commit reference"
From: Junio C Hamano @ 2023-10-24 22:26 UTC (permalink / raw)
To: Andrei Rybak; +Cc: git
In-Reply-To: <20231024195123.911431-1-rybak.a.v@gmail.com>
Andrei Rybak <rybak.a.v@gmail.com> writes:
> Documentation/SubmittingPatches informs the contributor that gitk's
> context menu command "Copy commit summary" can be used to obtain the
> conventional format of referencing existing commits. This command in
> gitk was renamed to "Copy commit reference" in commit [1], following
> implementation of Git's "reference" pretty format in [2].
>
> Update mention of this gitk command in Documentation/SubmittingPatches
> to its new name.
>
> [1] b8b60957ce (gitk: rename "commit summary" to "commit reference",
> 2019-12-12)
> [2] commit 1f0fc1d (pretty: implement 'reference' format, 2019-11-20)
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
> Documentation/SubmittingPatches | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks for a very well researched log message. Will queue.
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 0e2d3fbb9c..653bb2ad44 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -266,7 +266,7 @@ date)", like this:
> noticed that ...
> ....
>
> -The "Copy commit summary" command of gitk can be used to obtain this
> +The "Copy commit reference" command of gitk can be used to obtain this
> format (with the subject enclosed in a pair of double-quotes), or this
> invocation of `git show`:
^ permalink raw reply
* Re: [PATCH] send-email: move validation code below process_address_list
From: Junio C Hamano @ 2023-10-24 22:03 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
In-Reply-To: <xmqqmsw73cua.fsf@gitster.g>
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.
^ permalink raw reply
* Re: [PATCH] send-email: move validation code below process_address_list
From: Junio C Hamano @ 2023-10-24 21:55 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
In-Reply-To: <ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com>
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.
Here is how #58 fails (the last part of "cd t && sh t9001-*.sh -i -v").
expecting success of 9001.58 'In-Reply-To without --chain-reply-to':
clean_fake_sendmail &&
echo "<unique-message-id@example.com>" >expect &&
git send-email \
--from="Example <nobody@example.com>" \
--to=nobody@example.com \
--no-chain-reply-to \
--in-reply-to="$(cat expect)" \
--smtp-server="$(pwd)/fake.sendmail" \
$patches $patches $patches \
2>errors &&
# The first message is a reply to --in-reply-to
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
test_cmp expect actual &&
# Second and subsequent messages are replies to the first one
sed -n -e "s/^Message-ID: *\(.*\)/\1/p" msgtxt1 >expect &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
test_cmp expect actual &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
test_cmp expect actual
0001-Second.patch
0001-Second.patch
0001-Second.patch
(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
OK. Log says:
Sendmail: /usr/local/google/home/jch/w/git.git/t/trash directory.t9001-send-email/fake.sendmail -i nobody@example.com author@example.com one@example.com two@example.com committer@example.com
From: Example <nobody@example.com>
To: nobody@example.com
Cc: A <author@example.com>,
One <one@example.com>,
two@example.com,
C O Mitter <committer@example.com>
Subject: [PATCH 1/1] Second.
Date: Tue, 24 Oct 2023 21:52:27 +0000
Message-ID: <20231024215229.1787922-1-nobody@example.com>
X-Mailer: git-send-email 2.42.0-705-g1a1f985ecc
In-Reply-To: <unique-message-id@example.com>
References: <unique-message-id@example.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
OK. Log says:
Sendmail: /usr/local/google/home/jch/w/git.git/t/trash directory.t9001-send-email/fake.sendmail -i nobody@example.com author@example.com one@example.com two@example.com committer@example.com
From: Example <nobody@example.com>
To: nobody@example.com
Cc: A <author@example.com>,
One <one@example.com>,
two@example.com,
C O Mitter <committer@example.com>
Subject: [PATCH 1/1] Second.
Date: Tue, 24 Oct 2023 21:52:28 +0000
Message-ID: <20231024215229.1787922-2-nobody@example.com>
X-Mailer: git-send-email 2.42.0-705-g1a1f985ecc
In-Reply-To: <unique-message-id@example.com>
References: <unique-message-id@example.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
OK. Log says:
Sendmail: /usr/local/google/home/jch/w/git.git/t/trash directory.t9001-send-email/fake.sendmail -i nobody@example.com author@example.com one@example.com two@example.com committer@example.com
From: Example <nobody@example.com>
To: nobody@example.com
Cc: A <author@example.com>,
One <one@example.com>,
two@example.com,
C O Mitter <committer@example.com>
Subject: [PATCH 1/1] Second.
Date: Tue, 24 Oct 2023 21:52:29 +0000
Message-ID: <20231024215229.1787922-3-nobody@example.com>
X-Mailer: git-send-email 2.42.0-705-g1a1f985ecc
In-Reply-To: <unique-message-id@example.com>
References: <unique-message-id@example.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
--- expect 2023-10-24 21:52:29.115044899 +0000
+++ actual 2023-10-24 21:52:29.119045306 +0000
@@ -1 +1 @@
-<20231024215229.1787922-1-nobody@example.com>
+<unique-message-id@example.com>
not ok 58 - In-Reply-To without --chain-reply-to
#
# clean_fake_sendmail &&
# echo "<unique-message-id@example.com>" >expect &&
# git send-email \
# --from="Example <nobody@example.com>" \
# --to=nobody@example.com \
# --no-chain-reply-to \
# --in-reply-to="$(cat expect)" \
# --smtp-server="$(pwd)/fake.sendmail" \
# $patches $patches $patches \
# 2>errors &&
# # The first message is a reply to --in-reply-to
# sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
# test_cmp expect actual &&
# # Second and subsequent messages are replies to the first one
# sed -n -e "s/^Message-ID: *\(.*\)/\1/p" msgtxt1 >expect &&
# sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
# test_cmp expect actual &&
# sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
# test_cmp expect actual
#
1..58
^ permalink raw reply
* Re: [RESEND v2] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Oswald Buddenhagen @ 2023-10-24 21:31 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Junio C Hamano, Phillip Wood, Christian Couder,
Charvi Mendiratta, Marc Branchaud
In-Reply-To: <ZTamhY1sTpp1N6n+@nand.local>
On Mon, Oct 23, 2023 at 12:59:49PM -0400, Taylor Blau wrote:
>The line wrapping is a little odd: it looks like each sentence begins
>on a its own line.
>
this is indeed the case.
>Did you mean for there to be a visual separation between those
>sentences in the rendered doc?
>
no.
the idea is to keep the churn down in later edits, by making reflowing
the entire paragraph visibly unnecesary. i can change it if it's deemed
too weird.
regards
^ permalink raw reply
* Re: [PATCH 1/1] merge-file: add an option to process object IDs
From: brian m. carlson @ 2023-10-24 21:23 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Junio C Hamano, Elijah Newren, Phillip Wood
In-Reply-To: <CAPig+cT_yq-ke4RTpTdTTLsnJFxCtyGAP2K0mQ_S23jJYtUp=w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1793 bytes --]
On 2023-10-24 at 20:12:52, Eric Sunshine wrote:
> On Tue, Oct 24, 2023 at 3:58 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > git merge-file knows how to merge files on the file system already. It
> > would be helpful, however, to allow it to also merge single blobs.
> > Teach it an `--object-id` option which means that its arguments are
> > object IDs and not files to allow it to do so.
> >
> > Since we obviously won't be writing the data to the first argument,
> > either write to the object store and print the object ID, or honor the
> > -p argument and print it to standard out.
> >
> > We handle the empty blob specially since read_mmblob doesn't read it
> > directly, instead throwing an error, and otherwise users cannot specify
> > an empty ancestor.
> >
> > Signed-off-by: brian m. carlson <bk2204@github.com>
> > ---
> > diff --git a/builtin/merge-file.c b/builtin/merge-file.c
> > @@ -99,20 +116,29 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
> > if (ret >= 0) {
> > - const char *filename = argv[0];
> > - char *fpath = prefix_filename(prefix, argv[0]);
> > - FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
> > + if (object_id && !to_stdout) {
> > + struct object_id oid;
> > + if (result.size)
> > + write_object_file(result.ptr, result.size, OBJ_BLOB, &oid);
>
> Should this be caring about errors by checking the return value of
> write_object_file()?
Probably so. I think I saw write_object_file_prepare returned void and
misinterpreted that as write_object_file returning void.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: using oldest date when squashing commits
From: Johannes Sixt @ 2023-10-24 21:19 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood; +Cc: Oswald Buddenhagen, phillip.wood, git
In-Reply-To: <xmqqpm143p46.fsf@gitster.g>
Am 24.10.23 um 19:30 schrieb Junio C Hamano:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>> "fixup -c/-C" were conceived as a way to reword a commit message at
>> the same time as optionally fixing up the commit's content.
>
> Yup, it still is a "fix", meaning the identity and the spirit of the
> commit being fixed are unchanged.
That's a pitty, because that is not at all what *I* use "fixup -C" for.
To update the commit message, I use "squash" (or occasionally "reword").
I use "fixup -C" after the following events:
1. Commit unfinished changes for whatever reason. Usually the commit
message just says "WIP <topic>" because that's what it is.
2. Make a fixup commit for an earlier commit because doing the fixup now
gets it out of the way, and often delaying it until after the completed
change would cause merge conflicts.
3. Complete the WIP including the commit message.
I would now use "fixup -C" on commit 3, because its metadata reflects
reality more accurately than that of 1. Commit 3 often comes days after 1.
-- Hannes
^ permalink raw reply
* Re: [RESEND v2] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Oswald Buddenhagen @ 2023-10-24 21:19 UTC (permalink / raw)
To: Marc Branchaud
Cc: git, Junio C Hamano, Phillip Wood, Christian Couder,
Charvi Mendiratta
In-Reply-To: <e33f919d-1b6a-4944-ab5d-93ad0d323b68@xiplink.com>
On Tue, Oct 24, 2023 at 10:01:07AM -0400, Marc Branchaud wrote:
>I will only say that, I personally don't read man pages from
>start-to-end like a novel. I jump to the part that explains the thing
>I need to learn about. So I think your assumptions about what context
>a reader might have in mind when they see this text are invalid.
>
we are speaking about the context of a single paragraph, so that doesn't
seem like a relevant objection.
>> +The commit message for the folded commit is the concatenation of the
>> +message of the first commit with those of commits identified by "squash"
>
>s/message of the first commit/picked commit's message/
>
that does indeed sound better, but i think it's more confusing (and
potentially even more so when translated directly). i guess one could
use "pick'd commit's", but that's kind of ugly again.
>> +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 (having more than one of these is
>> +incorrect).
>
>As Phillip said, this is wrong. I agree with Phillip that the
>documentation should reflect the actual implementation, not what we hope
>the implementation might be some day.
>
there is also the middle ground of making it intentionally vague in
anticipation of a possible change. my current draft says "if multiple
are present, the last one takes precedence, but this should not be
relied upon".
>> +The first commit which contributes to the suggested commit message
>> also
>
>s/suggested/folded/ -- with "fixup -C" there is no "suggested" message.
>
that's a good point, but i want to emphasize the fact that it's the
pre-edit message, i.e., that trimming down the squashed message doesn't
change anything.
anyway, this part will be postponed to another contribution anyway (see
parallel thread).
thanks
^ permalink raw reply
* Re: [PATCH 11/12] builtin/show-ref: add new mode to check for reference existence
From: Eric Sunshine @ 2023-10-24 21:01 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <2f876e61dd36a8887a1286bb8db9fb6577c55c9b.1698152926.git.ps@pks.im>
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).
> + 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.
> 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).
> + 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"));
> + }
> +
> + 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;
> @@ -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).
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Uwe Kleine-König @ 2023-10-24 20:43 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Luben Tuikov, git, entwicklung
In-Reply-To: <89712aea-04fc-4775-afd4-afd3ca24ad01@amd.com>
[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]
Hello Michael,
On Tue, Oct 24, 2023 at 03:00:38PM -0400, Michael Strawbridge wrote:
> On 10/24/23 09:00, Uwe Kleine-König wrote:
> > On Fri, Oct 20, 2023 at 05:06:36PM -0400, Michael Strawbridge wrote:
> >> On 10/20/23 06:04, Uwe Kleine-König wrote:
> >>> On Fri, Oct 13, 2023 at 04:14:37PM +0200, Uwe Kleine-König wrote:
> >>>> $ git send-email --to 'A B <a@b.org>, C D <c@d.org>' lala.patch
> >>>> Use of uninitialized value $address in sprintf at /usr/lib/git-core/git-send-email line 1172.
> >>>> error: unable to extract a valid address from:
> >>>>
> >>>> This happens for me with git 2.42.0 and also on master (59167d7d09fd, "The seventeenth batch").
> Hm. I tried reproing with master (59167d7d09fd, "The seventeenth batch") but I don't seem to see an error:
> ```
> $ git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>' -1 --smtp-server="$(pwd)/fake.sendmail"
> [...]
I debugged a bit and if I do
mv .git/hooks/sendemail-validate .git/hooks/sendemail-validate.bak
git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>'
starts to work for me, too.
I'd guess the content of my sendemail-validate script doesn't matter
much, but for the record, it's:
#!/bin/sh
# installed by patatt install-hook
patatt sign --hook "${1}"
Does the problem reproduce on your end with a sendemail-validate script?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* bugreport
From: galo joel @ 2023-10-24 20:40 UTC (permalink / raw)
To: git
[-- Attachment #1.1: Type: text/plain, Size: 1 bytes --]
[-- Attachment #1.2: Type: text/html, Size: 26 bytes --]
[-- Attachment #2: git-bugreport-2023-10-24-2215.txt --]
[-- Type: text/plain, Size: 1442 bytes --]
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
execute as admin git bash and,(in cmd W10, same. open git-bash.exe as system-32).
try chmod 755, 777... does not work 'cause im user ($) and not admin (#)
Wha did you expect to happen? (Expected behavior)
change .sh to chmod 755 for execute bash
What happened instead? (Actual behavior)
nothing ._.
What's different between what you expected and what actually happened?
i expected a good sript in bash. now i'm sad
Anything else you want to add:
Please tell me what happen, maybe i'm wrong but chatGPT also no have idea why
i cannot be admin in my own laptop xD, or maybe i need some libraries that i didnot
install. I sell all my information so please help me to understand why does not work :)
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.42.0.windows.2
cpu: x86_64
built from commit: 2f819d1670fff9a1818f63b6722e9959405378e3
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 19045
compiler info: gnuc: 13.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe
[Enabled Hooks]
not run from a git repository - no hooks to show
^ permalink raw reply
* [PATCH] send-email: move validation code below process_address_list
From: Michael Strawbridge @ 2023-10-24 20:19 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Bagas Sanjaya, Git Mailing List
In-Reply-To: <393f598e-c7cd-4dc6-a221-9aed7ffcc2b1@amd.com>
From 09ea51d63cebdf9ff0c073ef86e21b4b09c268e5 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] 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.
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.
Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
git-send-email.perl | 48 ++++++++++++++++++++++-----------------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..a898dbc76e 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) {
@@ -2023,6 +1999,30 @@ sub process_file {
return 1;
}
+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};
+}
+
foreach my $t (@files) {
while (!process_file($t)) {
# user edited the file
--
2.42.0
^ permalink raw reply related
* Re: using oldest date when squashing commits
From: Oswald Buddenhagen @ 2023-10-24 20:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, phillip.wood, Johannes Sixt, git
In-Reply-To: <xmqqpm143p46.fsf@gitster.g>
On Tue, Oct 24, 2023 at 10:30:01AM -0700, Junio C Hamano wrote:
>Phillip Wood <phillip.wood123@gmail.com> writes:
>>>> Unfortunately "fixup -C" only copies the commit message not the
>>>> authorship
>>>
>>>> (that's usually a good thing
>>>>
>>> why? what would that be useful for?
>>> it seems rather counter-intuitive.
>>
>> In the same way that you do not want to change the author date when
>> using a fixup to move a small hunk from one commit to another most
>> users do not want to update the author information when they make a
>> small change to a commit message using "fixup -C"
>
>Exactly. [...]
>I wouldn't be able to use "rebase -i" to
>make typofixes to commits made out of received patches if the
>operation changes the authorship.
>
>> "fixup -c/-C" were conceived as a way to reword a commit message at
>> the same time as optionally fixing up the commit's content.
>
>Yup, it still is a "fix", meaning the identity and the spirit of the
>commit being fixed are unchanged. What it aims to achieve, how it
>implements the behaviour it wants to give its users, who thought of
>that change, all that are the same as the original.
>
ok, i think i finally got it. it would have never ocurred to me to make
a command for that - i just use "squash" and throw away the extra lines.
but i guess it sort of makes sense if you use rebase as a
non-interactive execution backend for instructions that are fully
determined long in advance by heaping commits at the end.
> It may be a nice addition to optionally allow users to use
> --reset-author (or better yet, --author="Na Me <a@dd.re.ss>") with
> "fixup"
>
that's kind of the opposite of what i'd want - the "pre-fixup" commit
already has the equivalent of that by virtue of being fresh. so it would
be more like --copy-author. but i'd go with adding -ca/-CA variants
instead, for brevity.
>but if the "-c" variant can be concluded with "commit --amend
>--reset-author" to achieve the same effect, that may be sufficient.
>
from the above follows that the equivalent of my original request would
be appending "exec git commit --amend -C <orig>" to the "pick
<pre-fixup>" + "fixup <orig>" commands. which is of course horrible, and
i'd never remember to actually do that. it will be hard enough to
retrain myself to use -CA instead of -C.
regards
^ permalink raw reply
* Re: [PATCH 1/1] merge-file: add an option to process object IDs
From: Eric Sunshine @ 2023-10-24 20:12 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Junio C Hamano, Elijah Newren, Phillip Wood
In-Reply-To: <20231024195655.2413191-2-sandals@crustytoothpaste.net>
On Tue, Oct 24, 2023 at 3:58 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> git merge-file knows how to merge files on the file system already. It
> would be helpful, however, to allow it to also merge single blobs.
> Teach it an `--object-id` option which means that its arguments are
> object IDs and not files to allow it to do so.
>
> Since we obviously won't be writing the data to the first argument,
> either write to the object store and print the object ID, or honor the
> -p argument and print it to standard out.
>
> We handle the empty blob specially since read_mmblob doesn't read it
> directly, instead throwing an error, and otherwise users cannot specify
> an empty ancestor.
>
> Signed-off-by: brian m. carlson <bk2204@github.com>
> ---
> diff --git a/builtin/merge-file.c b/builtin/merge-file.c
> @@ -99,20 +116,29 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
> if (ret >= 0) {
> - const char *filename = argv[0];
> - char *fpath = prefix_filename(prefix, argv[0]);
> - FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
> + if (object_id && !to_stdout) {
> + struct object_id oid;
> + if (result.size)
> + write_object_file(result.ptr, result.size, OBJ_BLOB, &oid);
Should this be caring about errors by checking the return value of
write_object_file()?
> + } else {
> + const char *filename = argv[0];
> + char *fpath = prefix_filename(prefix, argv[0]);
> + FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
> + if (!f)
> + ret = error_errno("Could not open %s for writing",
> + filename);
> + else if (result.size &&
> + fwrite(result.ptr, result.size, 1, f) != 1)
> + ret = error_errno("Could not write to %s", filename);
> + else if (fclose(f))
> + ret = error_errno("Could not close %s", filename);
> + free(fpath);
The non-"object-id" case cares about errors.
^ permalink raw reply
* Re: [PATCH 0/3] some send-email --compose fixes
From: Michael Strawbridge @ 2023-10-24 20:12 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Bagas Sanjaya, Git Mailing List
In-Reply-To: <20231023185152.GC1537181@coredump.intra.peff.net>
On 10/23/23 14:51, Jeff King wrote:
> On Fri, Oct 20, 2023 at 02:42:13PM -0700, Junio C Hamano wrote:
>
>>> So here's the fix in a cleaned up form, guided by my own comments from
>>> earlier. ;) I think this is actually all orthogonal to the patch you are
>>> working on, so yours could either go on top or just be applied
>>> separately.
>>>
>>> [1/3]: doc/send-email: mention handling of "reply-to" with --compose
>>> [2/3]: Revert "send-email: extract email-parsing code into a subroutine"
>>> [3/3]: send-email: handle to/cc/bcc from --compose message
>>
>> Nice.
>>
>> With the approach suggested to move the validation down to where the
>> necessary addresses are already all defined, Michael observed "whoa,
>> why am I getting stringified array ref?". If that is the only issue
>> in the approach, queuing these three patches first and then have
>> Michael's fix on top of them sounds like the cleanest thing to do.
Patch coming soon.
>
> I don't think it is even an issue in Michael's approach. I'd have to see
> his patch and how he tested it to be sure, but I suspect he was simply
> being extra careful to test nearby behavior and stumbled upon the
> ARRAY() bug. But the bug was there long before either of his patches.
>
Thank you for your patches Peff! I think it fixes the issue I was seeing.
I was trying to be extra careful with my testing. I had missed testing
--compose and also the multiple --to/cc/bcc examples before.
>> Will queue on top of v2.42.0 to help those who may want to backport
>> these to the maintenance track.
>
> So I think you could take my series on top of master (or 2.42.0), and
> eventually target 'master'. The bug it fixes is from 2017, so not
> urgent. The reading of "to" headers is a new feature.
>
> But the fix to move the validation around should probably go directly
> onto a8022c5f7b (send-email: expose header information to
> git-send-email's sendemail-validate hook, 2023-04-19) for use on maint.
> I guess maybe it is not that urgent anymore, as that regression is in
> v2.41, and we would not release anything along that maint track anymore,
> though.
>
> -Peff
^ permalink raw reply
* [PATCH 0/1] Object ID support for git merge-file
From: brian m. carlson @ 2023-10-24 19:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Elijah Newren, Phillip Wood
This series introduces an --object-id option to git merge-file such
that, instead of reading and writing from files on the system, it reads
from and writes to the object store using blobs. This is in use at
GitHub to produce conflict diffs when a merge fails, and it seems
generally useful, so I'm sending it here.
The only tricky piece is the fact that we have to special-case the empty
blob since otherwise it isn't handled correctly.
brian m. carlson (1):
merge-file: add an option to process object IDs
Documentation/git-merge-file.txt | 20 +++++++++++
builtin/merge-file.c | 58 +++++++++++++++++++++++---------
t/t6403-merge-file.sh | 58 ++++++++++++++++++++++++++++++++
3 files changed, 120 insertions(+), 16 deletions(-)
^ permalink raw reply
* [PATCH 1/1] merge-file: add an option to process object IDs
From: brian m. carlson @ 2023-10-24 19:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Elijah Newren, Phillip Wood
In-Reply-To: <20231024195655.2413191-1-sandals@crustytoothpaste.net>
From: "brian m. carlson" <bk2204@github.com>
git merge-file knows how to merge files on the file system already. It
would be helpful, however, to allow it to also merge single blobs.
Teach it an `--object-id` option which means that its arguments are
object IDs and not files to allow it to do so.
Since we obviously won't be writing the data to the first argument,
either write to the object store and print the object ID, or honor the
-p argument and print it to standard out.
We handle the empty blob specially since read_mmblob doesn't read it
directly, instead throwing an error, and otherwise users cannot specify
an empty ancestor.
Signed-off-by: brian m. carlson <bk2204@github.com>
---
Documentation/git-merge-file.txt | 20 +++++++++++
builtin/merge-file.c | 58 +++++++++++++++++++++++---------
t/t6403-merge-file.sh | 58 ++++++++++++++++++++++++++++++++
3 files changed, 120 insertions(+), 16 deletions(-)
diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt
index 7e9093fab6..45460f3916 100644
--- a/Documentation/git-merge-file.txt
+++ b/Documentation/git-merge-file.txt
@@ -12,6 +12,9 @@ SYNOPSIS
'git merge-file' [-L <current-name> [-L <base-name> [-L <other-name>]]]
[--ours|--theirs|--union] [-p|--stdout] [-q|--quiet] [--marker-size=<n>]
[--[no-]diff3] <current-file> <base-file> <other-file>
+'git merge-file' --object-id [-L <current-name> [-L <base-name> [-L <other-name>]]]
+ [--ours|--theirs|--union] [-q|--quiet] [--marker-size=<n>]
+ [--[no-]diff3] <current-oid> <base-oid> <other-oid>
DESCRIPTION
@@ -40,6 +43,10 @@ however, these conflicts are resolved favouring lines from `<current-file>`,
lines from `<other-file>`, or lines from both respectively. The length of the
conflict markers can be given with the `--marker-size` option.
+If `--object-id` is specified, exactly the same behavior occurs, except that
+instead of specifying what to merge as files, it is specified as a list of
+object IDs referring to blobs.
+
The exit value of this program is negative on error, and the number of
conflicts otherwise (truncated to 127 if there are more than that many
conflicts). If the merge was clean, the exit value is 0.
@@ -52,6 +59,14 @@ linkgit:git[1].
OPTIONS
-------
+--object-id::
+ Specify the contents to merge as blobs in the current repository instead of
+ files. In this case, the operation must take place within a valid repository.
++
+If the `-p` option is specified, the merged file (including conflicts, if any)
+goes to standard output as normal; otherwise, the merged file is written to the
+object store and the object ID of its blob is written to standard output.
+
-L <label>::
This option may be given up to three times, and
specifies labels to be used in place of the
@@ -93,6 +108,11 @@ EXAMPLES
merges tmp/a123 and tmp/c345 with the base tmp/b234, but uses labels
`a` and `c` instead of `tmp/a123` and `tmp/c345`.
+`git merge-file -p --object-id abc1234 def567 890abcd`::
+
+ combines the changes of the blob abc1234 and 890abcd since def567,
+ tries to merge them and writes the result to standard output
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index d7eb4c6540..d308434b8e 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -1,5 +1,8 @@
#include "builtin.h"
#include "abspath.h"
+#include "hex.h"
+#include "object-name.h"
+#include "object-store.h"
#include "config.h"
#include "gettext.h"
#include "setup.h"
@@ -31,10 +34,11 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
mmfile_t mmfs[3] = { 0 };
mmbuffer_t result = { 0 };
xmparam_t xmp = { 0 };
- int ret = 0, i = 0, to_stdout = 0;
+ int ret = 0, i = 0, to_stdout = 0, object_id = 0;
int quiet = 0;
struct option options[] = {
OPT_BOOL('p', "stdout", &to_stdout, N_("send results to standard output")),
+ OPT_BOOL(0, "object-id", &object_id, N_("use object IDs instead of filenames")),
OPT_SET_INT(0, "diff3", &xmp.style, N_("use a diff3 based merge"), XDL_MERGE_DIFF3),
OPT_SET_INT(0, "zdiff3", &xmp.style, N_("use a zealous diff3 based merge"),
XDL_MERGE_ZEALOUS_DIFF3),
@@ -71,8 +75,12 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
return error_errno("failed to redirect stderr to /dev/null");
}
+ if (object_id)
+ setup_git_directory();
+
for (i = 0; i < 3; i++) {
char *fname;
+ struct object_id oid;
mmfile_t *mmf = mmfs + i;
if (!names[i])
@@ -80,12 +88,21 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
fname = prefix_filename(prefix, argv[i]);
- if (read_mmfile(mmf, fname))
+ if (object_id) {
+ if (repo_get_oid(the_repository, argv[i], &oid))
+ ret = -1;
+ else if (!oideq(&oid, the_hash_algo->empty_blob))
+ read_mmblob(mmf, &oid);
+ else
+ read_mmfile(mmf, "/dev/null");
+ } else if (read_mmfile(mmf, fname)) {
ret = -1;
- else if (mmf->size > MAX_XDIFF_SIZE ||
- buffer_is_binary(mmf->ptr, mmf->size))
+ }
+ if (ret != -1 && (mmf->size > MAX_XDIFF_SIZE ||
+ buffer_is_binary(mmf->ptr, mmf->size))) {
ret = error("Cannot merge binary files: %s",
argv[i]);
+ }
free(fname);
if (ret)
@@ -99,20 +116,29 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
ret = xdl_merge(mmfs + 1, mmfs + 0, mmfs + 2, &xmp, &result);
if (ret >= 0) {
- const char *filename = argv[0];
- char *fpath = prefix_filename(prefix, argv[0]);
- FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
+ if (object_id && !to_stdout) {
+ struct object_id oid;
+ if (result.size)
+ write_object_file(result.ptr, result.size, OBJ_BLOB, &oid);
+ else
+ oidcpy(&oid, the_hash_algo->empty_blob);
+ printf("%s\n", oid_to_hex(&oid));
+ } else {
+ const char *filename = argv[0];
+ char *fpath = prefix_filename(prefix, argv[0]);
+ FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
- if (!f)
- ret = error_errno("Could not open %s for writing",
- filename);
- else if (result.size &&
- fwrite(result.ptr, result.size, 1, f) != 1)
- ret = error_errno("Could not write to %s", filename);
- else if (fclose(f))
- ret = error_errno("Could not close %s", filename);
+ if (!f)
+ ret = error_errno("Could not open %s for writing",
+ filename);
+ else if (result.size &&
+ fwrite(result.ptr, result.size, 1, f) != 1)
+ ret = error_errno("Could not write to %s", filename);
+ else if (fclose(f))
+ ret = error_errno("Could not close %s", filename);
+ free(fpath);
+ }
free(result.ptr);
- free(fpath);
}
if (ret > 127)
diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh
index 1a7082323d..2c92209eca 100755
--- a/t/t6403-merge-file.sh
+++ b/t/t6403-merge-file.sh
@@ -65,11 +65,30 @@ test_expect_success 'merge with no changes' '
test_cmp test.txt orig.txt
'
+test_expect_success 'merge with no changes with --object-id' '
+ git add orig.txt &&
+ git merge-file -p --object-id :orig.txt :orig.txt :orig.txt >actual &&
+ test_cmp actual orig.txt
+'
+
test_expect_success "merge without conflict" '
cp new1.txt test.txt &&
git merge-file test.txt orig.txt new2.txt
'
+test_expect_success 'merge without conflict with --object-id' '
+ git add orig.txt new2.txt &&
+ git merge-file --object-id :orig.txt :orig.txt :new2.txt >actual &&
+ git rev-parse :new2.txt >expected &&
+ test_cmp actual expected
+'
+
+test_expect_success 'can accept object ID with --object-id' '
+ git merge-file --object-id $(test_oid empty_blob) $(test_oid empty_blob) :new2.txt >actual &&
+ git rev-parse :new2.txt >expected &&
+ test_cmp actual expected
+'
+
test_expect_success 'works in subdirectory' '
mkdir dir &&
cp new1.txt dir/a.txt &&
@@ -138,6 +157,31 @@ test_expect_success "expected conflict markers" '
test_cmp expect.txt test.txt
'
+test_expect_success "merge with conflicts with --object-id" '
+ git add backup.txt orig.txt new3.txt &&
+ test_must_fail git merge-file -p --object-id :backup.txt :orig.txt :new3.txt >actual &&
+ sed -e "s/<< test.txt/<< :backup.txt/" \
+ -e "s/>> new3.txt/>> :new3.txt/" \
+ expect.txt >expect &&
+ test_cmp expect actual &&
+ test_must_fail git merge-file --object-id :backup.txt :orig.txt :new3.txt >oid &&
+ git cat-file blob "$(cat oid)" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "merge with conflicts with --object-id with labels" '
+ git add backup.txt orig.txt new3.txt &&
+ test_must_fail git merge-file -p --object-id \
+ -L test.txt -L orig.txt -L new3.txt \
+ :backup.txt :orig.txt :new3.txt >actual &&
+ test_cmp expect.txt actual &&
+ test_must_fail git merge-file --object-id \
+ -L test.txt -L orig.txt -L new3.txt \
+ :backup.txt :orig.txt :new3.txt >oid &&
+ git cat-file blob "$(cat oid)" >actual &&
+ test_cmp expect.txt actual
+'
+
test_expect_success "merge conflicting with --ours" '
cp backup.txt test.txt &&
@@ -256,6 +300,14 @@ test_expect_success 'binary files cannot be merged' '
grep "Cannot merge binary files" merge.err
'
+test_expect_success 'binary files cannot be merged with --object-id' '
+ cp "$TEST_DIRECTORY"/test-binary-1.png . &&
+ git add orig.txt new1.txt test-binary-1.png &&
+ test_must_fail git merge-file --object-id \
+ :orig.txt :test-binary-1.png :new1.txt 2> merge.err &&
+ grep "Cannot merge binary files" merge.err
+'
+
test_expect_success 'MERGE_ZEALOUS simplifies non-conflicts' '
sed -e "s/deerit.\$/deerit;/" -e "s/me;\$/me./" <new5.txt >new6.txt &&
sed -e "s/deerit.\$/deerit,/" -e "s/me;\$/me,/" <new5.txt >new7.txt &&
@@ -389,4 +441,10 @@ test_expect_success 'conflict sections match existing line endings' '
test $(tr "\015" Q <nolf.txt | grep "^[<=>].*Q$" | wc -l) = 0
'
+test_expect_success '--object-id fails without repository' '
+ empty="$(test_oid empty_blob)" &&
+ nongit test_must_fail git merge-file --object-id $empty $empty $empty 2>err &&
+ grep "not a git repository" err
+'
+
test_done
^ permalink raw reply related
* [PATCH] SubmittingPatches: call gitk's command "Copy commit reference"
From: Andrei Rybak @ 2023-10-24 19:51 UTC (permalink / raw)
To: git
Documentation/SubmittingPatches informs the contributor that gitk's
context menu command "Copy commit summary" can be used to obtain the
conventional format of referencing existing commits. This command in
gitk was renamed to "Copy commit reference" in commit [1], following
implementation of Git's "reference" pretty format in [2].
Update mention of this gitk command in Documentation/SubmittingPatches
to its new name.
[1] b8b60957ce (gitk: rename "commit summary" to "commit reference",
2019-12-12)
[2] commit 1f0fc1d (pretty: implement 'reference' format, 2019-11-20)
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
Documentation/SubmittingPatches | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 0e2d3fbb9c..653bb2ad44 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -266,7 +266,7 @@ date)", like this:
noticed that ...
....
-The "Copy commit summary" command of gitk can be used to obtain this
+The "Copy commit reference" command of gitk can be used to obtain this
format (with the subject enclosed in a pair of double-quotes), or this
invocation of `git show`:
--
2.42.0
^ permalink raw reply related
* Re: [PATCH 10/12] builtin/show-ref: explicitly spell out different modes in synopsis
From: Eric Sunshine @ 2023-10-24 19:39 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <adcfa7a6a9d8fb6f915faf77df52362544cd590e.1698152926.git.ps@pks.im>
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).
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`.
^ permalink raw reply
* Re: [PATCH 09/12] builtin/show-ref: ensure mutual exclusiveness of subcommands
From: Eric Sunshine @ 2023-10-24 19:25 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <d0a991cf4f892e73e4fd62ef3fdae3fa73277321.1698152926.git.ps@pks.im>
On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote:
> The git-show-ref(1) command has three different modes, of which one is
> implicit and the other two can be chosen explicitly by passing a flag.
> But while these modes are standalone and cause us to execute completely
> separate code paths, we gladly accept the case where a user asks for
> both `--exclude-existing` and `--verify` at the same time even though it
> is not obvious what will happen. Spoiler: we ignore `--verify` and
> execute the `--exclude-existing` mode.
>
> Let's explicitly detect this invalid usage and die in case both modes
> were requested.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> @@ -269,6 +269,9 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
> + if ((!!exclude_existing_opts.enabled + !!verify) > 1)
> + die(_("only one of --exclude-existing or --verify can be given"));
Somewhat recently, work was done to normalize this sort of message.
The result was to instead use the phrasing "options '%s' and '%s'
cannot be used together". See, for instance, 43ea635c35 (i18n:
refactor "foo and bar are mutually exclusive", 2022-01-05).
^ 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