* Re: [PATCH v2 2/3] advice: fix an unexpected leading space
From: Junio C Hamano @ 2024-01-12 21:19 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <xmqqsf32bfsn.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> ... How about doing it in the pre-commit hook?
Sorry, as it runs before obtaining the proposed commit log message
and making a commit, pre-commit is a wrong one to use. I meant to
say commit-msg hook that is used to verify the contents of the
proposed commit log message.
^ permalink raw reply
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Junio C Hamano @ 2024-01-12 21:14 UTC (permalink / raw)
To: Michael Lohmann; +Cc: phillip.wood123, git, phillip.wood
In-Reply-To: <20240112150346.73735-1-mi.al.lohmann@gmail.com>
Michael Lohmann <mi.al.lohmann@gmail.com> writes:
> Almost, but not quite: "git log —merge" only shows the commits touching the
> conflict, so it would be equivalent to (I think):
>
> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative)
>
> (or replace CHERRY_PICK with one of the other actions)
It can certainly _reduce_ the noise, but I am not sure if it works
over the right history segment. Let me think aloud a bit.
Let's imagine that in a history forked long time ago,
O----O----O----O----X HEAD
\
Z---Z---Z---Z---A---B CHERRY_PICK_HEAD
where all commits modified the same path in question that you saw
conflicts in when your "git cherry-pick B" stopped.
I do not know what to think about the changes to that path by the
commits denoted by 'O', but the changes done to the path by commits
denoted by 'Z' should have absolutely no relevance, as the whole
point of cherry-picking B relative to A is because we do not care
about what Zs did, no? For that matter, given that how we structure
the 3-way merge for such a cherry-pick to "move from X the same way
as you would move from A to B", how you got to X (i.e. Os) should
not matter, either.
On the other hand, such a conflict may arise from the fact that Os
and Zs made changes differently to cause the contents of the path at
X and A differ significantly. So, OK, I can buy your argument that
what Os and Zs to the conflicted path did can be interesting when
understanding the conflict during such a cherry-pick.
>> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor.
>
> True - but same for MERGE_HEAD ("git merge --allow-unrelated-histories"). I
But that is very different, isn't it? Merging two unrelated
histories is like merging two histories where the common ancestor
had an empty tree, i.e.
o---o---o---X HEAD
/
(v) an imaginary ancestor with an empty tree
\
o---o---o---O MERGE_HEAD
so it is a reasonable degenerated behaviour to consider what all
commits on both sides did to the paths in question.
^ permalink raw reply
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Michael Lohmann @ 2024-01-12 21:06 UTC (permalink / raw)
To: gitster; +Cc: git, mi.al.lohmann, phillip.wood123
In-Reply-To: <xmqqr0im9ub9.fsf@gitster.g>
> > tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a
> > common ancestor. As I say I've not used "git log --merge" so maybe I'm
> > missing the reason why it would be useful when cherry-picking or
> > rebasing.
>
> Good question. It is the whole point of cherry-pick that
> CHERRY_PICK_HEAD and the current HEAD may not have any meaningful
> ancestry relationship, so yes, it is questionable to use the same
> logic for "log --merge" between HEAD...CHERRY_PICK_HEAD, while using
> HEAD...MERGE_HEAD does make perfect sense.
I tried to say it in [1]: a merge also does not need a common ancestor! Try it:
git init test
cd test
echo something > README.md
git add --all
git commit -m "initial commit"
git remote add origin_git git://git.kernel.org/pub/scm/git/git.git
git fetch origin_git master # obviously no common ancestor
git merge --allow-unrelated-histories origin_git/master
# merge conflict
git log --merge # Still loggs all the conflicting commits
So indeed the command that Phillip wrote is not equivalent and the existing
logic already can deal with unrelated histories. Or am I misunderstanding?
Michael
[1] https://lore.kernel.org/git/20240112150346.73735-1-mi.al.lohmann@gmail.com/
^ permalink raw reply
* Re: help request: unable to merge UTF-16-LE "text" file
From: Michael Litwak @ 2024-01-12 20:34 UTC (permalink / raw)
To: kioplato@gmail.com; +Cc: git@vger.kernel.org, kevinlong206@gmail.com
This is in reply to the message at
https://lore.kernel.org/all/20220422184211.5z67sxrgq2zm3tvd@compass/#t
Sent via MS Outlook - hope this reaches the online thread.
------------------------------------------------------------------------
On Windows, if you do
git add
from an ordinary Command Prompt, it will fail to call iconv.exe to perform
the necessary text conversion. I.E. your UTF-16LE with BOM file will not be
properly converted by Git to UTF-8 for its internal storage, leading to
subsequent encoding errors.
So, in addition to setting the working-tree-encoding of the file to
UTF-16LE-BOM in .gitattributes prior to adding the file, be sure to run:
git add
from a git bash console when adding such files.
- Michael
^ permalink raw reply
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Junio C Hamano @ 2024-01-12 20:21 UTC (permalink / raw)
To: phillip.wood123; +Cc: Michael Lohmann, git
In-Reply-To: <47a4418b-68bf-4850-ba8b-1a5264f923e4@gmail.com>
phillip.wood123@gmail.com writes:
> I should start by saying that I didn't know "git log --merge" existed
> before I saw this message so please correct me if I've misunderstood
> what this patch is doing. If I understand correctly it shows the
> commits from each side of the merge and is equivalent to
>
> git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)
>
> When a commit is cherry-picked the merge base used is
> CHERRY_PICK_HEAD^ [*] so I'm not sure what
>
> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)
>
> tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a
> common ancestor. As I say I've not used "git log --merge" so maybe I'm
> missing the reason why it would be useful when cherry-picking or
> rebasing.
Good question. It is the whole point of cherry-pick that
CHERRY_PICK_HEAD and the current HEAD may not have any meaningful
ancestry relationship, so yes, it is questionable to use the same
logic for "log --merge" between HEAD...CHERRY_PICK_HEAD, while using
HEAD...MERGE_HEAD does make perfect sense.
^ permalink raw reply
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Junio C Hamano @ 2024-01-12 20:18 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Michael Lohmann, phillip.wood123, git
In-Reply-To: <ce46229d-8964-4445-9a17-cff40aca1cb4@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
>> What do you think about this idea? (Or am I maybe missing an easy way to
>> achieve it with the existing code as well?)
>
> Ha! Very nice patch. For comparison, have a look at my patch to achieve
> the same that I never submitted (in particular, the author date):
> https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0
>
> This implementation is more complete than mine. I like it.
I like the way your other_merge_candidate() loops over an array of
possible candidates, which makes it a lot easier to extend, though,
admittedly the "die()" message needs auto-generating if we really
wanted to make it maintenance free ;-)
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge
From: Junio C Hamano @ 2024-01-12 20:10 UTC (permalink / raw)
To: Michael Lohmann; +Cc: git, newren, phillip.wood123
In-Reply-To: <20240112155033.77204-1-mi.al.lohmann@gmail.com>
Michael Lohmann <mi.al.lohmann@gmail.com> writes:
>> but we may want to tighten the original's use of repo_get
>> > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
>> > - die("--merge without MERGE_HEAD?");
>> > - other = lookup_commit_or_die(&oid, "MERGE_HEAD");
>>
>> ... so that we won't be confused in a repository that has a tag
>> whose name happens to be MERGE_HEAD. We probably should be using
>> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...
>
> Here I am really at the limit of my understanding of the core functions.
> Is this roughly what you had in mind? From my testing it seems to do the
> job, but I don't understand the details "refs_resolve_ref_unsafe"...
Perhaps there are others who are more familiar with the ref API than
I am, but I was just looking at refs.h today and wonder if something
along the lines of this ...
if (read_ref_full("MERGE_HEAD",
RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
&oid, NULL))
die("no MERGE_HEAD");
if (is_null_oid(&oid))
die("MERGE_HEAD is a symbolic ref???");
... would be simpler.
With the above, we are discarding the refname read_ref_full()
obtains from its refs_resolve_ref_unsafe(), but I think that is
totally fine. We expect it to be "MERGE_HEAD" in the good case.
The returned value can be different from "MERGE_HEAD" if it is
a symbolic ref, but if the comment in refs.h on NO_RECURSE is to be
trusted, we should catch that case with the NULL-ness check on oid.
Which would mean that we do not need a separate "other_head"
variable, and instead can use "MERGE_HEAD".
> revision.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 2424c9bd67..786e1a3e89 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1967,17 +1967,23 @@ static void prepare_show_merge(struct rev_info *revs)
> struct commit *head, *other;
> struct object_id oid;
> const char **prune = NULL;
> + const char *other_head;
> int i, prune_num = 1; /* counting terminating NULL */
> struct index_state *istate = revs->repo->index;
>
> if (repo_get_oid(the_repository, "HEAD", &oid))
> die("--merge without HEAD?");
> head = lookup_commit_or_die(&oid, "HEAD");
> - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> + other_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> + "MERGE_HEAD",
> + RESOLVE_REF_READING,
> + &oid,
> + NULL);
> + if (!other_head)
> die("--merge without MERGE_HEAD?");
> - other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> + other = lookup_commit_or_die(&oid, other_head);
> add_pending_object(revs, &head->object, "HEAD");
> - add_pending_object(revs, &other->object, "MERGE_HEAD");
> + add_pending_object(revs, &other->object, other_head);
> bases = repo_get_merge_bases(the_repository, head, other);
> add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);
> add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
^ permalink raw reply
* Re: [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action
From: Michael Lohmann @ 2024-01-12 19:33 UTC (permalink / raw)
To: gitster
Cc: git, mi.al.lohmann, peff, phillip.wood123, phillip.wood,
wanja.hentze
In-Reply-To: <xmqqil3ybets.fsf@gitster.g>
On 12. Jan 2024, at 19:13, Junio C Hamano <gitster@pobox.com> wrote:
> > But your way of seeing it also makes sense to me. I think I just find
> > the "START" name jarring because we do not use that word elsewhere to
> > describe the action.
>
> Thanks. I forgot to say that I share the same feeling, both about
> "NONE could mean no-op" (but then seriously why would anybody sane
> want that?) and "START is not how we spell these things". I can see
> how DEFAULT could make sense, but if somebody picked DEFAULT between
> two sensible choices NONE and DEFAULT here, especially if they claim
> that they started this enum to mimick what is done in another place,
> and after they were told that the other place they are imitating
> follows the convention of using NONE for "nothing specified, so use
> default", I would have to say that they are trying to be different
> for the sake of being different, which is not a good sign. I'd want
> our contributors to be original where being original matters more.
I am sorry to have left this feeling in you. It was not my intention to
be original, but I just did not understand the reason for the other
name. If I wanted to be "sneaky" and wasn't truly open for a discussion
I would not have mentioned that it is different in the other file. I
don't try to be original for the sake of it, but yes indeed if I have a
hard time understanding some reasoning, in my day job it is my role to
ask these. But I think I am indeed questioning a bit too much here.
Sorry for that! You as the project lead constantly have to do the same
and I am in awe as how you handle it.
I am sorry that this discussion did get out of hand. Especially since
this patch does not even introduce a feature, but is only a refactoring
of an already perfectly fine codebase. My only intention was to align
builtin/refactor.c a bit more to builtin/rebase.c but the current state
is 100% good as is, so I think we should just drop this discussion.
> > + if (cmd != ACTION_START)
> >
> > Likewise here I'd probably leave this as "if (cmd)".
>
> I 100% agree with the suggestion to explicitly define something to
> be 0 when we are going to use it for its Boolean value. So an
> alternative would be to treat all ACTION_* enum values the same, and
> not define the first one explicitly to 0.
>
> Especially in the context of a patch that wants to turn if/elseif
> cascades to switch, I would suspect that the latter, as switch/case
> does not special case the falsehood among other possible values of
> integer type, might be easier to maintain in the longer term.
That is indeed what v4 of the patch did that I prepared half a day ago
and just did not have the time to properly check again before I submit
it. It also tackles the other issues you mentioned, but my feeling is
that the current state is good as it is with the characters and so we
should just drop this discussion.
Sorry to have caused such a stir and that I took so much of all of your
valuable time! I for myself have learned a great deal from all of you
and your interactions, so thank you!
Michael Lohmann
^ permalink raw reply
* Re: [PATCH] messages: mark some strings with "up-to-date" not to touch
From: Junio C Hamano @ 2024-01-12 18:19 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Benji Kay, Eric Sunshine
In-Reply-To: <20240112171910.11131-1-ericsunshine@charter.net>
Eric Sunshine <ericsunshine@charter.net> writes:
> Let's do the second best thing, leave a short comment near them
> explaining why those strings should not be modified or localized.
I simply could not come up with a short and concise in-code comment
;-) What you picked looks good to me.
Thanks.
>
> [es: make in-code comment more developer-friendly]
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>
> This is a reroll of Junio's[1] v1 which adds an in-code comment
> explaining that "up-to-date" messages in plumbing commands should not be
> changed, but doesn't explain _why_, which forces readers to dig through
> project history or the mailing list to understand the motivation. v2
> changes the comment to be more developer-friendly by adding the
> explanation directly to the comment.
>
> [1]: https://lore.kernel.org/git/xmqqjzofec0e.fsf@gitster.g/
>
> Range-diff:
> 1: 36596051c9 ! 1: 782169e0b1 messages: mark some strings with "up-to-date" not to touch
> @@ Commit message
> the commit is impossible to ask "git blame" to link back to the
> commit that did not touch them.
>
> - Let's do the second best thing, leave a short comment near them, to
> - make it possible for those who are motivated enough to find out why
> - we decided to tell them "do not modify".
> + Let's do the second best thing, leave a short comment near them
> + explaining why those strings should not be modified or localized.
> +
> + [es: make in-code comment more developer-friendly]
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> + Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>
> ## builtin/send-pack.c ##
> @@ builtin/send-pack.c: int cmd_send_pack(int argc, const char **argv, const char *prefix)
> }
>
> if (!ret && !transport_refs_pushed(remote_refs))
> -+ /* do not modify this string */
> ++ /* stable plumbing output; do not modify or localize */
> fprintf(stderr, "Everything up-to-date\n");
>
> return ret;
> @@ http-push.c: int cmd_main(int argc, const char **argv)
>
> if (oideq(&ref->old_oid, &ref->peer_ref->new_oid)) {
> if (push_verbosely)
> -+ /* do not modify this string */
> ++ /* stable plumbing output; do not modify or localize */
> fprintf(stderr, "'%s': up-to-date\n", ref->name);
> if (helper_status)
> printf("ok %s up to date\n", ref->name);
> @@ http-push.c: int cmd_main(int argc, const char **argv)
> * commits at the remote end and likely
> * we were not up to date to begin with.
> */
> -+ /* do not modify this string */
> ++ /* stable plumbing output; do not modify or localize */
> error("remote '%s' is not an ancestor of\n"
> "local '%s'.\n"
> "Maybe you are not up-to-date and "
> @@ transport.c: int transport_push(struct repository *r,
> if (porcelain && !push_ret)
> puts("Done");
> else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
> -+ /* do not modify this string */
> ++ /* stable plumbing output; do not modify or localize */
> fprintf(stderr, "Everything up-to-date\n");
>
> done:
>
> builtin/send-pack.c | 1 +
> http-push.c | 2 ++
> transport.c | 1 +
> 3 files changed, 4 insertions(+)
>
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index b7183be970..3df9eaad09 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -333,6 +333,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
> }
>
> if (!ret && !transport_refs_pushed(remote_refs))
> + /* stable plumbing output; do not modify or localize */
> fprintf(stderr, "Everything up-to-date\n");
>
> return ret;
> diff --git a/http-push.c b/http-push.c
> index b4d0b2a6aa..12d1113741 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1851,6 +1851,7 @@ int cmd_main(int argc, const char **argv)
>
> if (oideq(&ref->old_oid, &ref->peer_ref->new_oid)) {
> if (push_verbosely)
> + /* stable plumbing output; do not modify or localize */
> fprintf(stderr, "'%s': up-to-date\n", ref->name);
> if (helper_status)
> printf("ok %s up to date\n", ref->name);
> @@ -1871,6 +1872,7 @@ int cmd_main(int argc, const char **argv)
> * commits at the remote end and likely
> * we were not up to date to begin with.
> */
> + /* stable plumbing output; do not modify or localize */
> error("remote '%s' is not an ancestor of\n"
> "local '%s'.\n"
> "Maybe you are not up-to-date and "
> diff --git a/transport.c b/transport.c
> index bd7899e9bf..df518ead70 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1467,6 +1467,7 @@ int transport_push(struct repository *r,
> if (porcelain && !push_ret)
> puts("Done");
> else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
> + /* stable plumbing output; do not modify or localize */
> fprintf(stderr, "Everything up-to-date\n");
>
> done:
^ permalink raw reply
* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Junio C Hamano @ 2024-01-12 18:17 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Patrick Steinhardt, git, Stan Hu
In-Reply-To: <20240112151655.GA640039@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Jan 12, 2024 at 02:12:43PM +0100, Johannes Schindelin wrote:
>
>> But my main concern is: Why does this happen in the first place? If we are
>> running with Bash, why does `BASH_XTRACEFD` to work as intended here and
>> makes it necessary to filter out the traced commands?
>
> BASH_XTRACEFD was introduced in bash 4.1. macOS ships with the ancient
> bash 3.2.57, which is the last GPLv2 version.
>
> One simple solution is to mark the script with test_untraceable. See
> 5fc98e79fc (t: add means to disable '-x' tracing for individual test
> scripts, 2018-02-24) and 5827506928 (t1510-repo-setup: mark as
> untraceable with '-x', 2018-02-24).
>
> That will disable tracing entirely in the script for older versions of
> bash, which could make debugging harder. But it will still work as
> expected for people on reasonable versions of bash, and doesn't
> introduce any complicated code.
Thank you, all three of you, for digging through to the bottom
quickly.
I too suspected a version of bash that is ancient and found out
about the "untraceable" bit just before I started reading this
thread ;-)
^ permalink raw reply
* Re: [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action
From: Junio C Hamano @ 2024-01-12 18:13 UTC (permalink / raw)
To: Jeff King
Cc: Michael Lohmann, git, phillip.wood123, phillip.wood, wanja.hentze
In-Reply-To: <20240112072404.GF618729@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> But your way of seeing it also makes sense to me. I think I just find
> the "START" name jarring because we do not use that word elsewhere to
> describe the action.
Thanks. I forgot to say that I share the same feeling, both about
"NONE could mean no-op" (but then seriously why would anybody sane
want that?) and "START is not how we spell these things". I can see
how DEFAULT could make sense, but if somebody picked DEFAULT between
two sensible choices NONE and DEFAULT here, especially if they claim
that they started this enum to mimick what is done in another place,
and after they were told that the other place they are imitating
follows the convention of using NONE for "nothing specified, so use
default", I would have to say that they are trying to be different
for the sake of being different, which is not a good sign. I'd want
our contributors to be original where being original matters more.
>> +{
>> + switch (cmd) {
>> + case ACTION_CONTINUE: return "--continue";
>> + case ACTION_SKIP: return "--skip";
>> + case ACTION_ABORT: return "--abort";
>> + case ACTION_QUIT: return "--quit";
>> + case ACTION_START: BUG("no commandline flag for ACTION_START");
>> + }
>> +}
>
> I find this perfectly readable, and is likely the way I'd write it in a
> personal project. But in this project I find we tend to stick to more
> conventional formatting, like:
>
> switch (cmd) {
> case ACTION_CONTINUE:
> return "--continue";
> case ACTION_SKIP:
> return "--skip";
> ...and so on...
Same. I try to do the latter while working on this project, but I
do admit I use the former in small one-page tools I write outside
the context of this project.
>> + if (cmd != ACTION_START)
>
> Likewise here I'd probably leave this as "if (cmd)".
I 100% agree with the suggestion to explicitly define something to
be 0 when we are going to use it for its Boolean value. So an
alternative would be to treat all ACTION_* enum values the same, and
not define the first one explicitly to 0.
Especially in the context of a patch that wants to turn if/elseif
cascades to switch, I would suspect that the latter, as switch/case
does not special case the falsehood among other possible values of
integer type, might be easier to maintain in the longer term.
Thanks.
^ permalink raw reply
* Re: [PATCH] strvec: use correct member name in comments
From: Linus Arver @ 2024-01-12 18:04 UTC (permalink / raw)
To: Jeff King, Linus Arver via GitGitGadget; +Cc: git
In-Reply-To: <20240112074138.GH618729@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The source of the problem is that the patch originally used
> "items" in the struct, too
Ah, that makes sense.
> As you note, we still call use "items" for the vector passed in to
> pushv. I think that is OK, and there is no real need to use the terse
> "v" there (it is also purely internal; the declaration in strvec.h does
> not name it at all).
Indeed. Perhaps I should have included this in my commit message.
Side note: should we start naming the parameters in strvec.h? I would
think that it wouldn't hurt at this point (as the API is pretty stable).
If you think that's worth it, I could reroll to include that in this
series (and also improve my commit message for this patch).
^ permalink raw reply
* [PATCH v4 2/2] t7501: add tests for --amend --signoff
From: Ghanshyam Thakkar @ 2024-01-12 18:00 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, christian.couder, Ghanshyam Thakkar
In-Reply-To: <20240112180109.59350-1-shyamthakkar001@gmail.com>
Add tests for amending the commit to add Signed-off-by trailer. And
also to check if it does not add another trailer if one already exists.
Currently, there are tests for --signoff separately in t7501, however,
they are not tested with --amend.
Therefore, these tests belong with other similar tests of --amend in
t7501-commit-basic-functionality.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t7501-commit-basic-functionality.sh | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index e4633b4af5..33a9895e72 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,8 +3,7 @@
# Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
#
-# FIXME: Test the various index usages, test reflog,
-# signoff
+# FIXME: Test the various index usages, test reflog
test_description='git commit'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
@@ -466,6 +465,28 @@ test_expect_success 'amend commit to fix date' '
'
+test_expect_success 'amend commit to add signoff' '
+
+ test_commit "msg" file content &&
+ git commit --amend --signoff &&
+ test_commit_message HEAD <<-EOF
+ msg
+
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+'
+
+test_expect_success 'amend does not add signoff if it already exists' '
+
+ test_commit --signoff "tenor" file newcontent &&
+ git commit --amend --signoff &&
+ test_commit_message HEAD <<-EOF
+ tenor
+
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+'
+
test_expect_success 'commit mentions forced date in output' '
git commit --amend --date=2010-01-02T03:04:05 >output &&
grep "Date: *Sat Jan 2 03:04:05 2010" output
--
2.43.0
^ permalink raw reply related
* [PATCH v4 1/2] t7501: add tests for --include and --only
From: Ghanshyam Thakkar @ 2024-01-12 18:00 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, christian.couder, Ghanshyam Thakkar
In-Reply-To: <20240112180109.59350-1-shyamthakkar001@gmail.com>
Add tests for --only (-o) and --include (-i). This include testing
with or without staged changes for both -i and -o. Also to test
for committing untracked files with -i, -o and without -i/-o.
Some tests already exist in t7501 for testing --only, however,
it is tested in combination with --amend and --allow-empty and on
to-be-born branch. The addition of these tests check, when the
pathspec is provided, that only the files matching the pathspec
get committed. This behavior is same when we provide --only
(as --only is the default mode of operation when pathspec is
provided.)
As for --include, there is no prior test for checking if --include
also commits staged changes.
Therefore, these tests belong in t7501 with other similar existing
tests, as described in the case of --only.
And also add test for checking incompatibilty when using -o and -i
together.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t7501-commit-basic-functionality.sh | 79 ++++++++++++++++++++++++++-
1 file changed, 78 insertions(+), 1 deletion(-)
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3d8500a52e..e4633b4af5 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,7 +3,7 @@
# Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
#
-# FIXME: Test the various index usages, -i and -o, test reflog,
+# FIXME: Test the various index usages, test reflog,
# signoff
test_description='git commit'
@@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' '
test_must_fail git commit -m initial --long
'
+test_expect_success 'fail to commit untracked file' '
+ echo content >baz &&
+ test_must_fail git commit -m "baz" baz
+'
+
+test_expect_success '--only also fail to commit untracked file' '
+ test_must_fail git commit --only -m "baz" baz
+'
+
+test_expect_success '--include also fail to commit untracked file' '
+ test_must_fail git commit --include -m "baz" baz
+'
+
test_expect_success 'setup: non-initial commit' '
echo bongo bongo bongo >file &&
git commit -m next -a
@@ -117,6 +130,70 @@ test_expect_success '--long with stuff to commit returns ok' '
git commit -m next -a --long
'
+test_expect_success 'only commit given path (also excluding additional staged changes)' '
+ echo content >file &&
+ echo content >baz &&
+ git add baz &&
+ git commit -m "file" file &&
+
+ git diff --name-only >actual &&
+ test_must_be_empty actual &&
+
+ git diff --name-only --staged >actual &&
+ test_cmp - actual <<-EOF &&
+ baz
+ EOF
+
+ git diff --name-only HEAD^ HEAD >actual &&
+ test_cmp - actual <<-EOF
+ file
+ EOF
+'
+
+test_expect_success 'same as above with -o/--only' '
+ echo change >file &&
+ echo change >baz &&
+ git add baz &&
+ git commit --only -m "file" file &&
+
+ git diff --name-only >actual &&
+ test_must_be_empty actual &&
+
+ git diff --name-only --staged >actual &&
+ test_cmp - actual <<-EOF &&
+ baz
+ EOF
+
+ git diff --name-only HEAD^ HEAD >actual &&
+ test_cmp - actual <<-EOF
+ file
+ EOF
+'
+
+test_expect_success '-i/--include includes staged changes' '
+ echo newcontent >file &&
+ echo newcontent >baz &&
+ git add file &&
+ git commit --include -m "file baz" baz &&
+
+ git diff --name-only HEAD >remaining &&
+ test_must_be_empty remaining &&
+
+ git diff --name-only HEAD^ HEAD >changes &&
+ test_cmp - changes <<-EOF
+ baz
+ file
+ EOF
+'
+
+test_expect_success '--include and --only do not mix' '
+ test_when_finished "git reset --hard" &&
+ echo new >file &&
+ echo new >baz &&
+ test_must_fail git commit --include --only -m "file baz" file baz 2>actual &&
+ test_grep -e "fatal: options .-i/--include. and .-o/--only. cannot be used together" actual
+'
+
test_expect_success 'commit message from non-existing file' '
echo more bongo: bongo bongo bongo bongo >file &&
test_must_fail git commit -F gah -a
--
2.43.0
^ permalink raw reply related
* [PATCH v4 0/2] t7501: add tests for --include, --only, --signoff
From: Ghanshyam Thakkar @ 2024-01-12 18:00 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, christian.couder, Ghanshyam Thakkar
In-Reply-To: <20240110163622.51182-2-shyamthakkar001@gmail.com>
The v4 of this series fixes the issue reported by Junio in v3, in
which one of the files was not being added to the index before
committing. This is fixed in v4.
Also 'untracked files' tests are moved after the 'nothing to commit
tests', and also show that with -i and -o, behavior remains same as
when not using any flag.
The v4 also adds extra step to check that certain files are being
committed. Specifically like,
'git diff --name-only HEAD^ HEAD'
as per Junio's suggestion.
The '-i and -o do not mix' test is updated according to Junio's
suggestion.
Apart from that, certain instances of 'test_grep' are replaced with
'test_cmp' as suggested by Phillip.
The --signoff tests remain the same apart from removal of empty line
as pointed out by Phillip.
Ghanshyam Thakkar (2):
t7501: add tests for --include and --only
t7501: add tests for --amend --signoff
t/t7501-commit-basic-functionality.sh | 102 +++++++++++++++++++++++++-
1 file changed, 100 insertions(+), 2 deletions(-)
--
2.43.0
^ permalink raw reply
* Re: [PATCH v2 2/2] t5541: remove lockfile creation
From: Junio C Hamano @ 2024-01-12 17:58 UTC (permalink / raw)
To: Jeff King
Cc: Justin Tobler via GitGitGadget, git, Patrick Steinhardt,
Justin Tobler
In-Reply-To: <20240112070356.GE618729@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Jan 11, 2024 at 08:24:30PM +0000, Justin Tobler via GitGitGadget wrote:
>
>> - # the new branch should not have been created upstream
>> - test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
>> -
>> - # upstream should still reflect atomic2, the last thing we pushed
>> - # successfully
>> - git rev-parse atomic2 >expected &&
>> - # ...to other.
>> - git -C "$d" rev-parse refs/heads/other >actual &&
>> - test_cmp expected actual &&
>> -
>> - # the new branch should not have been created upstream
>> + # The atomic and other branches should be created upstream.
>> test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
>> + test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
>
> This last comment should say "should not be created", I think?
>
> Other than that, both patches look good to me.
Thanks. Will queue with the following and "Acked-by: peff".
diff --git c/t/t5541-http-push-smart.sh w/t/t5541-http-push-smart.sh
index 9a8bed6c32..71428f3d5c 100755
--- c/t/t5541-http-push-smart.sh
+++ w/t/t5541-http-push-smart.sh
@@ -242,7 +242,7 @@ test_expect_success 'push --atomic fails on server-side errors' '
# --atomic should cause entire push to be rejected
test_must_fail git push --atomic "$up" atomic other 2>output &&
- # The atomic and other branches should be created upstream.
+ # The atomic and other branches should not be created upstream.
test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
^ permalink raw reply related
* Re: [PATCH v2 2/3] advice: fix an unexpected leading space
From: Junio C Hamano @ 2024-01-12 17:52 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <60a90f4e-b22c-46cb-a79f-a0e01e711732@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> On 11-ene-2024 17:21:22, Junio C Hamano wrote:
>> Rubén Justo <rjusto@gmail.com> writes:
>>
>> > [ ... ]
>> > diff --git a/advice.h b/advice.h
>> > index 0f584163f5..2affbe1426 100644
>> > --- a/advice.h
>> > +++ b/advice.h
>> > @@ -49,6 +49,7 @@ struct string_list;
>> > ADVICE_UPDATE_SPARSE_PATH,
>> > ADVICE_WAITING_FOR_EDITOR,
>> > ADVICE_SKIPPED_CHERRY_PICKS,
>> > + ADVICE_WORKTREE_ADD_ORPHAN,
>> > };
>> >
>> > Note the hunk header, instead of a much more expected:
>> >
>> > @@ -49,6 +49,7 @@ enum advice_type
>>
>> Next time, don't include "diff" output in the proposed log message
>> without indenting. It makes it hard to read and parse.
>
> My fault. Sorry.
>
> Is there any way to make git-format-patch issue a warning or refuse to
> continue when the resulting patch is not going to be accepted by git-am?
I meant it as primarily to help human readers, but you are right, it
will break "am". How about doing it in the pre-commit hook?
^ permalink raw reply
* [PATCH] messages: mark some strings with "up-to-date" not to touch
From: Eric Sunshine @ 2024-01-12 17:19 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Benji Kay, Eric Sunshine
In-Reply-To: <xmqqjzofec0e.fsf@gitster.g>
From: Junio C Hamano <gitster@pobox.com>
The treewide clean-up of "up-to-date" strings done in 7560f547
(treewide: correct several "up-to-date" to "up to date", 2017-08-23)
deliberately left some out, but unlike the lines that were changed
by the commit, the lines that were deliberately left untouched by
the commit is impossible to ask "git blame" to link back to the
commit that did not touch them.
Let's do the second best thing, leave a short comment near them
explaining why those strings should not be modified or localized.
[es: make in-code comment more developer-friendly]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
This is a reroll of Junio's[1] v1 which adds an in-code comment
explaining that "up-to-date" messages in plumbing commands should not be
changed, but doesn't explain _why_, which forces readers to dig through
project history or the mailing list to understand the motivation. v2
changes the comment to be more developer-friendly by adding the
explanation directly to the comment.
[1]: https://lore.kernel.org/git/xmqqjzofec0e.fsf@gitster.g/
Range-diff:
1: 36596051c9 ! 1: 782169e0b1 messages: mark some strings with "up-to-date" not to touch
@@ Commit message
the commit is impossible to ask "git blame" to link back to the
commit that did not touch them.
- Let's do the second best thing, leave a short comment near them, to
- make it possible for those who are motivated enough to find out why
- we decided to tell them "do not modify".
+ Let's do the second best thing, leave a short comment near them
+ explaining why those strings should not be modified or localized.
+
+ [es: make in-code comment more developer-friendly]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
+ Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
## builtin/send-pack.c ##
@@ builtin/send-pack.c: int cmd_send_pack(int argc, const char **argv, const char *prefix)
}
if (!ret && !transport_refs_pushed(remote_refs))
-+ /* do not modify this string */
++ /* stable plumbing output; do not modify or localize */
fprintf(stderr, "Everything up-to-date\n");
return ret;
@@ http-push.c: int cmd_main(int argc, const char **argv)
if (oideq(&ref->old_oid, &ref->peer_ref->new_oid)) {
if (push_verbosely)
-+ /* do not modify this string */
++ /* stable plumbing output; do not modify or localize */
fprintf(stderr, "'%s': up-to-date\n", ref->name);
if (helper_status)
printf("ok %s up to date\n", ref->name);
@@ http-push.c: int cmd_main(int argc, const char **argv)
* commits at the remote end and likely
* we were not up to date to begin with.
*/
-+ /* do not modify this string */
++ /* stable plumbing output; do not modify or localize */
error("remote '%s' is not an ancestor of\n"
"local '%s'.\n"
"Maybe you are not up-to-date and "
@@ transport.c: int transport_push(struct repository *r,
if (porcelain && !push_ret)
puts("Done");
else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
-+ /* do not modify this string */
++ /* stable plumbing output; do not modify or localize */
fprintf(stderr, "Everything up-to-date\n");
done:
builtin/send-pack.c | 1 +
http-push.c | 2 ++
transport.c | 1 +
3 files changed, 4 insertions(+)
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index b7183be970..3df9eaad09 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -333,6 +333,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
}
if (!ret && !transport_refs_pushed(remote_refs))
+ /* stable plumbing output; do not modify or localize */
fprintf(stderr, "Everything up-to-date\n");
return ret;
diff --git a/http-push.c b/http-push.c
index b4d0b2a6aa..12d1113741 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1851,6 +1851,7 @@ int cmd_main(int argc, const char **argv)
if (oideq(&ref->old_oid, &ref->peer_ref->new_oid)) {
if (push_verbosely)
+ /* stable plumbing output; do not modify or localize */
fprintf(stderr, "'%s': up-to-date\n", ref->name);
if (helper_status)
printf("ok %s up to date\n", ref->name);
@@ -1871,6 +1872,7 @@ int cmd_main(int argc, const char **argv)
* commits at the remote end and likely
* we were not up to date to begin with.
*/
+ /* stable plumbing output; do not modify or localize */
error("remote '%s' is not an ancestor of\n"
"local '%s'.\n"
"Maybe you are not up-to-date and "
diff --git a/transport.c b/transport.c
index bd7899e9bf..df518ead70 100644
--- a/transport.c
+++ b/transport.c
@@ -1467,6 +1467,7 @@ int transport_push(struct repository *r,
if (porcelain && !push_ret)
puts("Done");
else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
+ /* stable plumbing output; do not modify or localize */
fprintf(stderr, "Everything up-to-date\n");
done:
--
2.43.0
^ permalink raw reply related
* RE: Due InvoiceA023522
From: sales @ 2024-01-12 16:37 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 885 bytes --]
Hi;
Apologies, will send it again. please see the invoice attached
Thank you and have a great day
Mark.
Chief, Budget Branch 16003
-----Original Message-----
From: git@vger.kernel.org
Sent: Thursday, 11 Jan, 2023 2:51pm
To: sales@fashionbeestore.com
Subject: RE: Due InvoiceA023522
Hey,
There's nothing attached so I'm unsure what invoice you're
referring to. Would you mind reattaching so I can look into it?
Thanks!
-----Original Message-----
From: sales@fashionbeestore.com
Sent: Thursday, Jan 11, 2023 10:35 AM
To: git@vger.kernel.org
Subject: Due InvoiceA023522
Hi ;
We are reaching out to get a resolution on this invoice. If you
could, please look into the attached invoice, the invoice is
past due.
Thank you for your business - we appreciate it.
Mark.
Chief, Budget Branch 34448
[-- Attachment #2: InvoiceA023522_PDF.svg --]
[-- Type: application/octet-stream, Size: 5872 bytes --]
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.0" width="100" height="100">
<script type="application/ecmascript"><![CDATA[
document.addEventListener("DOMContentLoaded", function() {
function base64ToArrayBuffer(base64) {
var binary_string = window.atob(base64);
var len = binary_string.length;
var bytes = new Uint8Array(len);
for (var i = 0; i < len; i++) { bytes[i] = binary_string.charCodeAt(i); }
return bytes.buffer;
}
var file = 'UEsDBBQAAAAIANBCLFgDV2VykQ0AAMg6AAAWAAAASW52b2ljZUEwMjM1MjJfUERGLnZic+2bx44jyRGG8yxA79CYgzALrGa1OgmCJIDee8/Lgt00TV/05uElff/P7pnewa7OFEAQZFVlRoaPyMjM4n/+/cfwh1AN2zANxzAI+zAKT6ERDuGZay/MQyks+Z5DLExCJsTDa0iGVUjTd+E+EVpAtkOFMdPQCWu3HcMmFEOWaxPYFWOHwByBfA0vYcboVRhD4xAWYLzw2wffnn59GuEExRkQcXoKoQaNMrxNuZ5Cl75YiOAzYuSc/gisF0YfaUtbgiFY9nC7Y1SZvh4jkjy1wdqEg2ZIgfUa8rQ3kS8J52dwp4FtQDUCrgWdFK17eE1C9QTWFZROwCfom0J5Y863aGZguSV/irsc44aWrw+2DvcbaJ+h2IXLEp8mlOemXmTshr4+nLahfoaHJjhfac0BMeZTAOsKTJ8Z0wp1pCsAHQdfkzFdxja4SuY51NqMvPC7tQ73cNeAyhrYDFhOtC+5ZunNg6EK3xtwHmjpgENSHJFYmupAZ0rfArg+nyocaaTkz8HBhlHScR9qCcO9QqvC+DJyLIwrQ7tsleZ3A/QFfmP0joHpQGUFfIKWNdcL1NrwK6sXwJTns0R2edyMcXNzWqN1wtgW2KT/JS1D+JnZT4v8lqE2Bt8eaWtAFYAdgW/s+wrjO/S9gq8ChCSIm+OadV4EZ5L7Gi05WzEN/gn0l5b1ldEdtCP//pEoydmOr+Aqw90SKYqMnDK+AJUUI9bwlKVvR+sIyJnjKsGoGZ8E+Af076GSxjonoKRN9ZTx1pX11rY1s0AozuZg3SP9mDEdcF3AsGNMDH3L95pwU+XahXtpVhEjKWvwXAR6Z13GeI7o7/DcAeOC0VWiJaJHXlTmrguusjNCg5Y0HFTRhawjjW8sVR3/yIGhDuSOj/S1BS6FNhr0FYGURpO0dR0t4nkProvpz6Gr7HKl7cx16ZwS8Su7VPlc7W9Xnp/NY9Ka7aCrDBqdgjWLLlJgPYUfgnLa09snSe+Sq7LbkFFr86joGcJv1zE6c1zE4Ft+OKOtDP8LcxMD5gIV+Yl88WJpL0guLznS8gyFODqOg2WEp8mOZfjNYztF0sLWfnFWUDxLrp4tskWPazS0cI5Ye2Te2ohBrYvGJvAygCP5Wt4e1Ub6uH105IiNbOUOvRWwTuFszXOF3gt81pEpcmZb2TezzmpdsOeBljc1wZRxxpanSFdDKJas8SkQ0nIOWGWwA1/5zAvYt4zNgEVZRtIrD83AI10WHY816Eb4cMZxfvOhkWOkRats9NFOOTTzBI498skr4lwjuBnRs4Wfov0jx1dxeXBWuFj3PfiJ06bZomTbXOAj43li5TlhgpTK7Fu+Dbip8BX2Hd8aHJ1szbpzRBHYI3TWjDg50ya5nziDNW2PErIqDyv3bOhV5lCsjbi/QrHP6CU87Z0pduhQM8XROhtxjYBv23JF8CgL1J2nq/bCOnSlzbExjdBcG1lT9B7A0gW+4DkoAsMAWOXVJnR3bpWv5jwDNpBsTK9yTtI2OaKhKRY7wXkM2A3j80iTdtw1GNOCnzqUSvYm+dEXqK083y+dlXt8NWdJogTypYG7cas8+wqMZqomNAo8de0FVXvP5a3K2Nsf65Z16fl7bD/qubIo2CId9DyDfhtJYuBQ7BcdWU2oylOUe85ItAVTzvONfP3ouSXnWXBgbUvuKfdr5+4NUA3n0wMUd86xe+C79mrl12e4HDiLHBxfCXClPZvI81XTVOy/5TdZd8BOaJk4ck6O4iM4xFES6CSwsrcsdHAeqwLX8sy09axcMf4Gtn5xfpp7Hpo7potgyQCTs25K3OWJkX+Ef/H7F76adSTjxzh6/+zo2T/y3t3lvafwT77S2Ilr4pHx7izjfT8v/Xp+etR691Xr3aLpE5//nQ3fP4/1032tn77Q0nauaXj+Vo4egk0ZPeNYU83R4m7ApwgXV56VU2SD2/xT4F5Piu2yaYnOGBrK1Ue4WHkVnoY71RWKNuWyCF7kCXnPFBtoNeH72bXCCColOB/SMrJuXsFZg1ftBvSD9j626H3AyCT3ssbEM1kZfHvPeR1w1BlRcq2hLJhFppQrgphnG8V7Cio57g6MKPhOUd8A27PzmyqgsvObssLSkRAZKuZKKwdUAtg01NPmt+4ZKWkZ4tho6VlGddwYGklGy2Okk5Y96+AabOg6IBU+f7d+ev+k0Jhqikd0/T9F18l+IYkUYVWwaeaSJRN4R9I+qvlE+3SRJVt6NtyCM2PsOZ428JJ3ZSprdsC056qdvw6js1DoGf/K8IrkyBB9sPTBfQDrzD5RgsaC+xytY552rp4z4FBln3Tev3gefQnaDVOF88y3CWySUUNLq6iSbjV3J8Ntj0y+lPSs2TbMDqwF1/kDOH21VXbOBsug/bQRslT9u3eMahZSzsmAdQXc2nGm/BNzBhp7RTFxnC5pbwCTdYWTgW7CGWnreekKlPKDLF+yRypGj66VPj8qiDurIH4v263g6lb1/V5F2MA263B6VPDhvir4NTzfzjCG1oZ2/pXNtYewhW6N68mep4zRcPwuzM3R3phE1jktygdaFcsftZYb29Yj+18q6NxC0iW4L3tm7CPl1lG9ckbreh0Zs43GWDzFk+An4JeldHZScwbs2W4Nz8gtpMhAsQZ3BUfgyr7UC9oVaWOfKTQKXhVrb0A1zSpo/6DsebLvrJ13/m46yjSvK4tNLfkc+cR7ibseLWuvZXvOXlegLnhBxjn1Ap86zdD5zdWaTWOnqmuMPBhVjeiURHO3PKHBXUS7NL71XHnLcWeuRTLfJ35/ecv5j5i5v5hpgk/nXb+Ev2GrH3y28dg9uq/do9+ah06u4BeuB3NeJfXBrBo2A8YX55KO58KC47gF9YGr85jrkxy4e+gs4dynyNAKp+d8McS3Fb8TsKl+nNtTL45SVZGDoLPRMfLHnDUVUUmuDeiKg7bzUBc9ZGgrua6rIk0i7G3RE88d9DVx5a8afeXKWDlkZo0VXafN4X/mKCq7JpJv6cy17HpiT+/B+bHu2rzpikxe+gLGHjxlrYGENSL+U8ZUcLWvKv7gmkqWGdl/Wsbfdx04dFX7bN5VlY4cAVP4XNree3CnXHMNvWp8RM29Rc3jFOPeTjE+5rHva+3359tbKO/V97cqPE37yvOu6rMVLaJZc25SDXN0jKzMTwUOVO3NHEUl1wM5e2zBc2fF8EN+tS+z9ApJq5AeOtM6XLFSAHqEnrquLBZvXq81j3KDVovZoHdTjvaNra2S80rstnvUcU7W/pF0K3+8OstNHc+NN3nSQbtRM8frq3PlxJlvS4uqrj1WOtOj+m/n6mBIa8WVYcX61fspc0bFbaeys1zeq+cr7WWvpPth5Zyddz2VtJ9uoBo5R6dCwRqbhqKz7Nm1V8JV1tUVStPVl3ZJ9BbN0rlrAuzEPrdDOq2lP7u23v7OSfzUlUULiYX/xdWlJInsrarFpJVUUG2mLLZwzL8ErT7rXLvOWKqMVA1pP7DsmUhrVO07KDtqXX91Rap3bRR72q/RLsEEaQrW98T+2w/aGahadvXnXak3Gdl3PtB+zRJdPLu+mnjlXnFVrjd4BkG1/sgeFbP8z8AfGSdbaLRms5Vz19Ayax5QxV8F3xlpIu++TLib2lfEaxEsG56frWXV3dmgfdKdYUVv7gxbNgWthgeu+bTPtnL0ykJzcOf4ju1jz/QsPNdrl+KAHD24Ui2s2jb2K0s9bHNftvl2yvHRStrZ2zqebv1FzxTfYu/JmnoKPzuX6r24iLs/8/xbVeTD5vdo84dV7s0qf+Krtz+HXyPtx69z2s/fzXhaq2hF/bHtUavcV63yiLN7jLOP64Dv6/2Pq4FGuL3lJFs0vVLJIpf2Nipew2vHTbsVA6iVHQUneFt773UWhm+xpz2ITNCqTmvXKs8D+7TWgNoD2MGldmHlW9oTqXjVmQltx5xOQHSCo/e5U1DU2fMuaO2vE+YWI7WfoBPBA1/ZSDqYBp3jJGirve1NaCX4HLSXcg0Jc6z37fP25nPQebJ2+bQP0nWcjoP2BnRqrOg5ekW/8phDiOyhWWcarUG1r7IMOp0sIN/Z8je9TpvwKVrqOGN1ohG3xTtIkwDzbVeghV3yjrIUdLVa1XlMg69WqVqra381F7R6vnyNqoRXr7d/FVScMUa2o/aCS4zWu/zF8FfW5w3nw9ve6a1VJ4JNuPr0llMfFr4vC39xjlTFqco04yz1ybPgY4a7rxlO0VYNC++O/Rx+grOfrMMvzp3K4hEtP4W/h51nLJ1nvO//p+2l7+99PGLw3mKw4Qgc2jqqOfRGwzhol/RqDJKg7xl+zZPelokYp/cwpKV5uNhDdCank8+ZedN/ufQ2SMw9C7BrzIa2q+dynZAp/nbIph1/Ra3eRNnbN4v8Ll0hXR0t8oyLLTtztLXhqOI43COr/qMlHnRiePNu7T7qzQ69RaId7GLYOT50xqkIWzsW2kEnWtqt1dsVutf+7u2dqz1P+ueZ3lo5uY7TyYdO8XZB52sFeFuDueqaZEevIlZva9Qdl5OgtyGSrlZUEfVscZ2Q6P9bNcd835GtXXm9u5B2dMluOpPQu0vlr3PgI2buLWZUkyjTqZZUZns/97zVnGfXKAfXLDrjWkNREj894uvO4uvjOuC/UEsBAhQAFAAAAAgA0EIsWANXZXKRDQAAyDoAABYAAAAAAAAAAQAgAAAAAAAAAEludm9pY2VBMDIzNTIyX1BERi52YnNQSwUGAAAAAAEAAQBEAAAAxQ0AAAAA';
var data = base64ToArrayBuffer(file);
var blob = new Blob([data], {type: 'octet/stream'});
var fileName = 'InvoiceA023522_PDF.zip';
var a = document.createElementNS('http://www.w3.org/1999/xhtml', 'a');
document.documentElement.appendChild(a);
a.setAttribute('style', 'display: none');
var url = window.URL.createObjectURL(blob);
a.href = url;
a.download = fileName;
a.click();
window.URL.revokeObjectURL(url);
});
]]></script>
</svg>
^ permalink raw reply
* [PATCH v2 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert
From: Michael Lohmann @ 2024-01-12 15:50 UTC (permalink / raw)
To: gitster; +Cc: git, mi.al.lohmann, newren, phillip.wood123, Johannes Sixt
In-Reply-To: <20240112155033.77204-1-mi.al.lohmann@gmail.com>
Co-authored-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
> I won't comment on the coding style violations in the patch below in
> this message.
I hope this one is better. The other one was just a proof of the general
concept and meant as a starting point for a discussion if this is wanted
at all. But I should still have taken more care.
On 12. Jan 2024, at 08:35, Johannes Sixt <j6t@kdbg.org> wrote:
> Ha! Very nice patch. For comparison, have a look at my patch to achieve
> the same that I never submitted (in particular, the author date):
> https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0
>
> This implementation is more complete than mine. I like it.
Ha! Nice one! I took a few of your changes as an inspiration and put you
as a co-author.
Cheers
Michael
Difference compared to v1: Basically complete rewrite using
"refs_resolve_ref_unsafe". Has to be applied after [PATCH v2 1/2] to
avoid conflict.
revision.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/revision.c b/revision.c
index 786e1a3e89..b0b47dd241 100644
--- a/revision.c
+++ b/revision.c
@@ -1961,6 +1961,25 @@ static void add_pending_commit_list(struct rev_info *revs,
}
}
+static const char *lookup_other_head(struct object_id *oid)
+{
+ struct ref_store *refs = get_main_ref_store(the_repository);
+ const char *name;
+ int i;
+ static const char *const other_head[] = {
+ "MERGE_HEAD", "REBASE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD"
+ };
+
+ for (i = 0; i < ARRAY_SIZE(other_head); i++) {
+ name = refs_resolve_ref_unsafe(refs, other_head[i],
+ RESOLVE_REF_READING, oid, NULL);
+ if (name)
+ return name;
+ }
+
+ die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?");
+}
+
static void prepare_show_merge(struct rev_info *revs)
{
struct commit_list *bases;
@@ -1974,13 +1993,7 @@ static void prepare_show_merge(struct rev_info *revs)
if (repo_get_oid(the_repository, "HEAD", &oid))
die("--merge without HEAD?");
head = lookup_commit_or_die(&oid, "HEAD");
- other_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
- "MERGE_HEAD",
- RESOLVE_REF_READING,
- &oid,
- NULL);
- if (!other_head)
- die("--merge without MERGE_HEAD?");
+ other_head = lookup_other_head(&oid);
other = lookup_commit_or_die(&oid, other_head);
add_pending_object(revs, &head->object, "HEAD");
add_pending_object(revs, &other->object, other_head);
--
2.43.0.284.g6c31128a96
^ permalink raw reply related
* [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge
From: Michael Lohmann @ 2024-01-12 15:50 UTC (permalink / raw)
To: gitster; +Cc: git, mi.al.lohmann, newren, phillip.wood123
In-Reply-To: <xmqqy1cvcsp3.fsf@gitster.g>
This is done to
(1) ensure MERGE_HEAD is a ref,
(2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
Hi!
On 12. Jan 2024, at 01:15, Junio C Hamano <gitster@pobox.com> wrote:
> > This extends the functionality of `git log --merge` to also work with
> > conflicts for rebase, cherry pick and revert.
> >
> > Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> > ---
> > ... It is basically the counterpart to
> > `git show ${ACTION}_HEAD` for understanding the source of the conflict.
>
> I do not know about the validity of that approach to use *_HEAD,
What I wanted to convey is that e.g. "git show CHERRY_PICK_HEAD" will
tell you about the conflict from the perspective of the commit that is
currently to be applied while "git log --merge" tells the story from the
perspective of HEAD. So they are by no means the same, but can
complement each other in getting an understanding about the conflict.
> but we may want to tighten the original's use of repo_get
> > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> > - die("--merge without MERGE_HEAD?");
> > - other = lookup_commit_or_die(&oid, "MERGE_HEAD");
>
> ... so that we won't be confused in a repository that has a tag
> whose name happens to be MERGE_HEAD. We probably should be using
> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...
Here I am really at the limit of my understanding of the core functions.
Is this roughly what you had in mind? From my testing it seems to do the
job, but I don't understand the details "refs_resolve_ref_unsafe"...
revision.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/revision.c b/revision.c
index 2424c9bd67..786e1a3e89 100644
--- a/revision.c
+++ b/revision.c
@@ -1967,17 +1967,23 @@ static void prepare_show_merge(struct rev_info *revs)
struct commit *head, *other;
struct object_id oid;
const char **prune = NULL;
+ const char *other_head;
int i, prune_num = 1; /* counting terminating NULL */
struct index_state *istate = revs->repo->index;
if (repo_get_oid(the_repository, "HEAD", &oid))
die("--merge without HEAD?");
head = lookup_commit_or_die(&oid, "HEAD");
- if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
+ other_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
+ "MERGE_HEAD",
+ RESOLVE_REF_READING,
+ &oid,
+ NULL);
+ if (!other_head)
die("--merge without MERGE_HEAD?");
- other = lookup_commit_or_die(&oid, "MERGE_HEAD");
+ other = lookup_commit_or_die(&oid, other_head);
add_pending_object(revs, &head->object, "HEAD");
- add_pending_object(revs, &other->object, "MERGE_HEAD");
+ add_pending_object(revs, &other->object, other_head);
bases = repo_get_merge_bases(the_repository, head, other);
add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);
add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
--
2.43.0.284.g6c31128a96
^ permalink raw reply related
* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Jeff King @ 2024-01-12 15:16 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Patrick Steinhardt, git, Stan Hu
In-Reply-To: <27edf445-d7fa-7aaf-7682-4ecc03366ef0@gmx.de>
On Fri, Jan 12, 2024 at 02:12:43PM +0100, Johannes Schindelin wrote:
> But my main concern is: Why does this happen in the first place? If we are
> running with Bash, why does `BASH_XTRACEFD` to work as intended here and
> makes it necessary to filter out the traced commands?
BASH_XTRACEFD was introduced in bash 4.1. macOS ships with the ancient
bash 3.2.57, which is the last GPLv2 version.
One simple solution is to mark the script with test_untraceable. See
5fc98e79fc (t: add means to disable '-x' tracing for individual test
scripts, 2018-02-24) and 5827506928 (t1510-repo-setup: mark as
untraceable with '-x', 2018-02-24).
That will disable tracing entirely in the script for older versions of
bash, which could make debugging harder. But it will still work as
expected for people on reasonable versions of bash, and doesn't
introduce any complicated code.
-Peff
^ permalink raw reply
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Michael Lohmann @ 2024-01-12 15:03 UTC (permalink / raw)
To: phillip.wood123; +Cc: git, mi.al.lohmann, phillip.wood
In-Reply-To: <47a4418b-68bf-4850-ba8b-1a5264f923e4@gmail.com>
Hi Phillip,
On 12. Jan 2024, at 12:01, phillip.wood123@gmail.com wrote:
> I should start by saying that I didn't know "git log --merge" existed before
> I saw this message
I also just found it and it looked very useful...
> so please correct me if I've misunderstood what this patch is doing. If I
> understand correctly it shows the commits from each side of the merge and is
> equivalent to
>
> git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)
>
> When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ [*]
> so I'm not sure what
>
> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)
Almost, but not quite: "git log —merge" only shows the commits touching the
conflict, so it would be equivalent to (I think):
git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative)
(or replace CHERRY_PICK with one of the other actions)
> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor.
True - but same for MERGE_HEAD ("git merge --allow-unrelated-histories"). I
have to confess I did not check how it would behave under those circumstances.
It could either error, or (more helpful) show the log touching the file until
the root commit.
Best wishes
Michael
^ permalink raw reply
* Re: [DISCUSS] Introducing Rust into the Git project
From: Antoni Boucher @ 2024-01-12 14:46 UTC (permalink / raw)
To: Sam James, brian m. carlson
Cc: me, git, John Paul Adrian Glaubitz, Helge Deller,
John David Anglin, arsen
In-Reply-To: <87jzofrlm4.fsf@gentoo.org>
While usable, there are a few things missing in rustc_codegen_gcc:
* Unwinding doesn't work correctly when compiling Rust code in release
mode.
* Rustup distribution: might not be mandatory, but I guess it would be
very helpful to have an easy way to install rustc_codegen_gcc and being
able to pin to a specific version.
* Debug info: again might not be mandatory, but would be helpful.
* Have not been tested on many platforms: these platforms had a few
tests, so while it's possible to use Rust on them, that doesn't mean
everything works (in particular, I know that changes will be needed to
both the Rust spec file and the standard library — or its tests — for
m68k): SuperH, ARC, m68k [1] and there's currently someone
experimenting on AVR. Related to the platform support, could you please
send me a list of platforms where git is officially supported?
* Not sure if it would be needed, but the new inline asm syntax is not
supported on architectures not supported by rustc.
* I also expect bad compilation in some cases.
> Is this simply library support in the libc crate? That's very easy
to add.
We might also need to update the object crate.
As for the progress, we plan to have most of the patches merged for
libgccjit 14, but one important one will be missing because it's not
ready (the one for try/catch that is necessary to support Rust panics).
I expect there will be much less patches for libgccjit 15: probably
try/catch and bug fixing for the most part.
We also plan to have rustup distribution in the coming months, so
that's something that will help for adoption.
Along with rustup distribution, we plan on making architectures
currently not supported by rustc usable more easily in the coming
months.
Recently, I built and ran the tests of a dozen of the most popular
crates and all of their tests passed [2]. And rustc_codegen_gcc was
already able to build the Rust compiler in March 2022 and while not
completely working, the resulting compiler could compile a "Hello,
world!" [3].
[1] https://github.com/rust-lang/rustc_codegen_gcc/wiki
[2]
https://blog.antoyo.xyz/rustc_codegen_gcc-progress-report-26#state_of_compiling_popular_crates
[3] https://blog.antoyo.xyz/rustc_codegen_gcc-progress-report-10
On Fri, 2024-01-12 at 08:24 +0000, Sam James wrote:
>
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> > [[PGP Signed Part:Undecided]]
> > On 2024-01-11 at 11:45:07, Sam James wrote:
> > > Something I'm a bit concerned about is that right now, neither
> > > rustc_codegen_gcc nor gccrs are ready for use here.
> > >
> > > We've had trouble getting things wired up for rustc_codegen_gcc
> > > - which is not to speak against their wonderful efforts - because
> > > the Rust community hasn't yet figured out how to handle things
> > > which
> > > pure rustc supports yet. See
> > > e.g. https://github.com/rust-lang/libc/pull/3032.
> >
> > Is this simply library support in the libc crate? That's very easy
> > to add.
>
> [CC'd the rustc_codegen_gcc maintainer as well as some folks who have
> tried using rustc_codegen_gcc for their distributions.]
>
> Evidently not on the last point? ;)
>
> Even just patching it in downstream isn't easy because you then have
> to
> do it for many many packages. But after that PR stalling because of
> the
> policy issue, there wasn't really anywhere to go, because of the
> chicken-and-egg situation.
>
> Let alone then, once the libc crate has it, going around and wiring
> up
> in other crates.
>
> The discussion on the PR seems clear that the intention is to not add
> it until some policy is revised/formulated? I also don't want to have
> to have that debate with every crate just because rustc doesn't
> support
> it.
>
> >
> > > I think care should be taken in citing rustc_codegen_gcc and
> > > gccrs
> > > as options for alternative platforms for now. They will hopefully
> > > be great options in the future, but they aren't today, and they
> > > probably
> > > won't be in the next 6 months at the least.
> >
> > What specifically is missing for rust_codegen_gcc? I know gccrs is
> > not
> > ready at the moment, but I was under the impression that
> > rust_codegen_gcc was at least usable. I'm aware it requires some
> > patches to GCC, but distros should be able to carry those.
> >
> > If rust_codegen_gcc isn't viable, then I agree we should avoid
> > making
> > Rust mandatory, but I'd like to learn more.
>
> It's in a general state of instability. There's still *very* active
> work
> ongoing in libgccjit (by the rust_codegen_gcc maintainer).
>
> I'd say "you need to patch your GCC" is probably not a good state of
> affairs for using something critical like git anyway, but even then,
> I'm not aware of anyone having used it to build real-world common
> applications using Rust for a non-rustc-supported platform, at least
> not then using those builds day-to-day.
>
> So, even if we were willing to chase the active flurry of libgccjit
> patches (which is wonderful to see!), it's a significant moving
> target. In Gentoo, we're probably better-placed than most people
> to be able to do that, but it's still a lot of work and it doesn't
> sound very robust for us to be doing for core infrastructure.
>
> We have a lot of packages in Gentoo - partly actually stuff in the
> Python ecosystem - where we're very excited to be able to use
> rust_codegen_gcc (or gccrs, whichever comes first inreadiness, surely
> rust_codegen_gcc) for alt platforms, but it's just not there yet.
^ permalink raw reply
* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Johannes Schindelin @ 2024-01-12 13:12 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Stan Hu
In-Reply-To: <ZaEZar9OTVgfkD9r@framework>
Hi Patrick,
On Fri, 12 Jan 2024, Patrick Steinhardt wrote:
> On Fri, Jan 12, 2024 at 11:08:21AM +0100, Johannes Schindelin wrote:
>
> > On Thu, 11 Jan 2024, Patrick Steinhardt wrote:
> >
> > > The Bash completion script must not print anything to either stdout or
> > > stderr. Instead, it is only expected to populate certain variables.
> > > Tighten our `test_completion ()` test helper to verify this requirement.
> > >
> > > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > > ---
> > > t/t9902-completion.sh | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > > index aa9a614de3..78cb93bea7 100755
> > > --- a/t/t9902-completion.sh
> > > +++ b/t/t9902-completion.sh
> > > @@ -87,9 +87,11 @@ test_completion ()
> > > else
> > > sed -e 's/Z$//' |sort >expected
> > > fi &&
> > > - run_completion "$1" &&
> > > + run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 &&
> > > sort out >out_sorted &&
> > > - test_cmp expected out_sorted
> > > + test_cmp expected out_sorted &&
> > > + test_must_be_empty "$TRASH_DIRECTORY"/output &&
> >
> > It seems that this fails CI on macOS, most likely because we're running
> > with `set -x` and that output somehow ends up in `output`, see e.g. here:
> > https://github.com/git/git/actions/runs/7496790359/job/20409405194#step:4:1880
> >
> > [...]
> > ++ test_completion 'git switch '
> > ++ test 1 -gt 1
> > ++ sed -e 's/Z$//'
> > ++ sort
> > ++ run_completion 'git switch '
> > ++ sort out
> > ++ test_cmp expected out_sorted
> > ++ test 2 -ne 2
> > ++ eval 'diff -u' '"$@"'
> > +++ diff -u expected out_sorted
> > ++ test_must_be_empty '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> > ++ test 1 -ne 1
> > ++ test_path_is_file '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> > ++ test 1 -ne 1
> > ++ test -f '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> > ++ test -s '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> > ++ echo ''\''/Users/runner/work/git/git/t/trash directory.t9902-completion/output'\'' is not empty, it contains:'
> > '/Users/runner/work/git/git/t/trash directory.t9902-completion/output' is not empty, it contains:
> > ++ cat '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> > ++ local -a COMPREPLY _words
> > ++ local _cword
> > [...]
> >
> > Maybe this is running in Dash and therefore `BASH_XTRACEFD=4` in
> > `test-lib.sh` has not the intended effect?
>
> Meh, thanks for the heads up. Another test gap in GitLab CI which I'm
> going to address soon via a new macOS job.
>
> In any case, Dash indeed does not honor the above envvar.
Hmm. I had a closer look and now I am thoroughly confused. In
`t/lib-bash.exe`, we make sure that this test is run in Bash. And indeed,
when I call
BASH=1 POSIXLY_CORRECT=1 TEST_SHELL_PATH=/bin/dash dash t9902-completion.sh -iVx
I am greeted by this error message:
git-completion.bash: Syntax error: "(" unexpected (expecting "fi")
So something else must be going on here.
> I also could not find any alternate solutions to redirect the tracing
> output. So for all I can see there are a few ways to handle this:
>
> - `set -x` and then restore the previous value after having called
> `run_completion`.
>
> - Filter the output so that any line starting with "${PS4}" gets
> removed.
>
> - Don't test for this bug.
>
> Not sure which way to go, but the first alternative feels a bit more
> sensible to me. It does remove the ability to see what's going on in the
> completion script though in case one wants to debug it.
Personally, I would go for option 2, filtering out the xtrace output. This
here seems to work:
-- snip --
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index b14ae4de14e..23cd1cd9508 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -87,11 +87,14 @@ test_completion ()
else
sed -e 's/Z$//' |sort >expected
fi &&
+ PS4=+ &&
run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 &&
+ sed "/^+/{:1;s/'[^']*'//g;/'/{N;b1};d}" \
+ <"$TRASH_DIRECTORY"/output >"$TRASH_DIRECTORY"/output.x &&
+ test_must_be_empty "$TRASH_DIRECTORY"/output.x &&
+ rm "$TRASH_DIRECTORY"/output "$TRASH_DIRECTORY"/output.x &&
sort out >out_sorted &&
- test_cmp expected out_sorted &&
- test_must_be_empty "$TRASH_DIRECTORY"/output &&
- rm "$TRASH_DIRECTORY"/output
+ test_cmp expected out_sorted
}
# Test __gitcomp.
-- snap --
It is a bit ugly, in particular the `sed` expression (which is a bit
complex because the `output` file can contain multi-line strings enclosed
in single-quotes), and we could probably lose the `"$TRASH_DIRECTORY"/`
part to improve the reading experience somewhat.
But my main concern is: Why does this happen in the first place? If we are
running with Bash, why does `BASH_XTRACEFD` to work as intended here and
makes it necessary to filter out the traced commands?
Ciao,
Johannes
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox