* Re: [PATCH] rebase -i: allow a comment after a "break" command
From: Elijah Newren @ 2023-01-14 2:47 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason,
Phillip Wood via GitGitGadget, git, Olliver Schinagl,
Johannes Schindelin, Phillip Wood, Christian Couder
In-Reply-To: <xmqqwn5q1960.fsf@gitster.g>
On Fri, Jan 13, 2023 at 12:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > Of course, the same applies to edit/squash/fixup/reword, though if I
> > could go back in time...(warning, long tangent coming)...I would make
> > it so those four directives did not accept any commit ID argument.
> > Only "pick" and "reset" would accept a commit ID. Instead, today's
> > "edit X" would be two commands ("pick X" followed by either "break" or
> > "edit"), "fixup X" would be "pick X" + "fixup", and "reword X" would
> > be "pick X" + "reword". That'd help users understand rebase state
> > much better (it's so easy for users to get confused by whether they
> > should be using `git commit --amend` vs. `git rebase --continue` and I
> > think this is partially to blame, though there's other changes we
> > could make to help with that dichotomy as well). The separate
> > directives would also make it much easier to figure out how to both
> > fixup and edit a single commit in the same rebase (pick the commit,
> > then add a fixup directive, then an edit directive).
>
> Intriguing, and I feel sad that it probably is too late for all of
> the above X-<.
Yeah, I know. One more thing for the "if we had a time machine" list...
> > In fact, "squash
> > X" could just be discarded as superfluous, since it's just "pick X" +
> > "fixup" + "reword" (or we could keep squash as an abbreviation for
> > both "fixup" + "reword").
>
> IIUC, your "fixup" is
>
> git reset --soft HEAD^
> git commit --amend --no-edit
>
> i.e. discard the log message from "fixup" and use only its tree, and
> your "reword" is
>
> git commit --amend --edit
>
> so "pick X" + "fixup" + "reword" would not be quite usable as a
> replacement of our "squash X" (or your "pick X" + "squash"), I am
> afraid. You'd want the log message from "X" as well as "X^" to
> edit the replacement of X^.
Oh, good point. So, in that alternate world we'd still need a squash directive.
^ permalink raw reply
* Re: [PATCH v2] doc: fix non-existent config name
From: Junio C Hamano @ 2023-01-14 1:33 UTC (permalink / raw)
To: muzimuzhi Z; +Cc: Clemens Buchacher, git
In-Reply-To: <CAEg0tHSZi22RUBREJB=Cfy6O72cicv9FTkgo_Z=gvGRdPK1acw@mail.gmail.com>
muzimuzhi Z <muzimuzhi@gmail.com> writes:
> From c879cb10f61afc361c484267f498d5815bc1b932 Mon Sep 17 00:00:00 2001
> From: muzimuzhi <muzimuzhi@gmail.com>
The "author identity" is taken from this line (or from the e-mail
header), and it should be identical to your sign-off.
> Date: Mon, 9 Jan 2023 06:37:47 +0800
> Subject: [PATCH v2] doc: fix non-existent config name
In general, there shouldn't be a reason to include the above four
lines in your message body. An exception is "From:" to override the
author identity when you cannot send your e-mail using the same name
as what is used on your sign-off. See Discussion section of "git am"
manual page.
> Replace non-existent `branch.<name>.fetch` to `remote.<repository>.fetch`, in
> the first example in `git-fetch` doc, which was introduced in
> d504f6975d (modernize fetch/merge/pull examples, 2009-10-21).
>
> Rename placeholder `<name>` to `<repository>`, to be consistent with all other
> uses in git docs, except that `git-config.txt` uses `remote.<name>.fetch` in
> its "Variables" section.
>
> Also add missing monospace markups.
>
> Signed-off-by: Yukai Chou <muzimuzhi@gmail.com>
Perfect. Will queue.
> Changes compared to PATCH v1:
> - Update commit reference in a non-shallow clone, resulting in longer
> <abbrev-hash>
Great.
> diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
> index 63d9569e16..fba66f1460 100644
> --- a/Documentation/git-fetch.txt
> +++ b/Documentation/git-fetch.txt
> @@ -251,10 +251,10 @@ EXAMPLES
> $ git fetch origin
> ------------------------------------------------
> +
> -The above command copies all branches from the remote refs/heads/
> -namespace and stores them to the local refs/remotes/origin/ namespace,
> -unless the branch.<name>.fetch option is used to specify a non-default
> -refspec.
> +The above command copies all branches from the remote `refs/heads/`
> +namespace and stores them to the local `refs/remotes/origin/` namespace,
> +unless the `remote.<repository>.fetch` option is used to specify a
> +non-default refspec.
>
> * Using refspecs explicitly:
> +
^ permalink raw reply
* Re: [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Junio C Hamano @ 2023-01-14 1:17 UTC (permalink / raw)
To: Strawbridge, Michael; +Cc: git@vger.kernel.org, Tuikov, Luben
In-Reply-To: <20230110211452.2568535-3-michael.strawbridge@amd.com>
"Strawbridge, Michael" <Michael.Strawbridge@amd.com> writes:
> +It takes these command line arguments:
> +1. the name of the file that holds the e-mail to be sent.
> +2. the name of the file that holds the SMTP headers to be used.
> +
> +The hook doesn't need to support multiple header names (for example only Cc
> +is passed).
I think you meant, by "multiple header names", "header names spelled
in different cases".
That may be a correct statement, but is more or less a useless one
that does not help hook writers. Different people spell these
headers in different capitalization (for example, your message came
with "CC:" to various people, not "Cc:"), so the hook MUST know
which case the feature adds to its input, if it chooses not to
support different cases like "Cc:", "cc:", and "CC:". IOW, "only Cc
is passed" is not something they need to hear as a mear example.
They need to be told what headers are given to them and in what
capitalization for all headers in the input to them.
> However, it does need to understand that lines beginning with
> +whitespace belong to the previous header. The header information follows
> +the same format as the confirmation given at the end of send-email.
I suspect that many people (including me) disable the confirmation
and to them, the above description would not help.
In general, documentation should not depend on the reader having an
access to an environment where they can readily run commands and see
their output.
Thanks.
^ permalink raw reply
* [PATCH v2] doc: fix non-existent config name
From: muzimuzhi Z @ 2023-01-14 1:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Clemens Buchacher, git
In-Reply-To: <xmqqo7r248x9.fsf@gitster.g>
From c879cb10f61afc361c484267f498d5815bc1b932 Mon Sep 17 00:00:00 2001
From: muzimuzhi <muzimuzhi@gmail.com>
Date: Mon, 9 Jan 2023 06:37:47 +0800
Subject: [PATCH v2] doc: fix non-existent config name
Replace non-existent `branch.<name>.fetch` to `remote.<repository>.fetch`, in
the first example in `git-fetch` doc, which was introduced in
d504f6975d (modernize fetch/merge/pull examples, 2009-10-21).
Rename placeholder `<name>` to `<repository>`, to be consistent with all other
uses in git docs, except that `git-config.txt` uses `remote.<name>.fetch` in
its "Variables" section.
Also add missing monospace markups.
Signed-off-by: Yukai Chou <muzimuzhi@gmail.com>
---
Changes compared to PATCH v1:
- Use real name in sign-off
- Update commit reference in a non-shallow clone, resulting in longer
<abbrev-hash>
- Use word "non-existent" in commit message uniformly
- Rebase to latest master a38d39a4c5 (The sixth batch, 2023-01-08)
Documentation/git-fetch.txt | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index 63d9569e16..fba66f1460 100644
--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -251,10 +251,10 @@ EXAMPLES
$ git fetch origin
------------------------------------------------
+
-The above command copies all branches from the remote refs/heads/
-namespace and stores them to the local refs/remotes/origin/ namespace,
-unless the branch.<name>.fetch option is used to specify a non-default
-refspec.
+The above command copies all branches from the remote `refs/heads/`
+namespace and stores them to the local `refs/remotes/origin/` namespace,
+unless the `remote.<repository>.fetch` option is used to specify a
+non-default refspec.
* Using refspecs explicitly:
+
--
2.39.0
^ permalink raw reply related
* Re: BUG: git grep behave oddly with alternatives
From: René Scharfe @ 2023-01-13 23:03 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, Jeff King
Cc: Marco Nenciarini, git, Junio C Hamano
In-Reply-To: <5f6908c8-3f84-92a3-b9d1-8689c500c1e9@web.de>
Am 13.01.23 um 18:24 schrieb René Scharfe:
> Am 13.01.23 um 09:28 schrieb Ævar Arnfjörð Bjarmason:
>>
>> - Just mandate PCRE for Mac OS, then map -E to -P. We could do this with
>> the pcre2convert() API and PCRE2_CONVERT_POSIX_EXTENDED flag,
>> i.e. PCRE's own "translate this to ERE".
>>
>> But Perl/PCRE syntax is already a superset of ERE syntax, minus things
>> like (*VERB), (?: etc., which people are unlikely to write without
>> intending to get the PCRE semantics.
>
> PCRE2_CONVERT_POSIX_EXTENDED is a flag for pcre2_pattern_convert(),
> which allows converting an ERE into a PCRE2 regex
> (https://pcre.org/current/doc/html/pcre2convert.html). So this feature
> allows using PCRE for every kind of regexes, right?
>
> There may be feature differences for EREs between libc on macOS, glibc
> and/or GAWK, but I'm not aware of any complaints so far. So I currently
> don't see the need for that.
Thought that maybe this could help us matching NUL characters and fix the
TODO in t7815, because PCRE2 can handle binary files just fine. But no:
pcre2_pattern_convert() uses (NUL*) in its result, meaning that NUL is
treated as a line separator and . (dot) won't match it. And it's still
experimental according to the documentation link I mentioned above.
René
^ permalink raw reply
* Re: [PATCH v6 2/2] attr: add flag `--source` to work with tree-ish
From: Junio C Hamano @ 2023-01-13 22:19 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, phillip.wood, sunshine, Toon Claes
In-Reply-To: <57f5957127ce3be2621445806f2f00857ac8b1aa.1673521102.git.karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> diff --git a/attr.h b/attr.h
> index 3fb40cced0..f4a2bedd68 100644
> --- a/attr.h
> +++ b/attr.h
> @@ -1,6 +1,8 @@
> #ifndef ATTR_H
> #define ATTR_H
>
> +#include "hash.h"
You only want "struct object_id" declared. Why include the whole
file?
^ permalink raw reply
* Re: [PATCH] t6426: fix TODO about making test more comprehensive
From: Andrei Rybak @ 2023-01-13 22:09 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Elijah Newren
In-Reply-To: <pull.1462.git.1673584084761.gitgitgadget@gmail.com>
On 13/01/2023 05:28, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> t6426.7 (a rename/add testcase) long had a TODO/FIXME comment about
> how the test could be improved (with some commented out sample code
> that had a few small errors), but those improvements were blocked on
> other changes still in progress. The necessary changes were put in
> place years ago but the comment was forgotten. Remove and fix the
> commented out code section and finally remove the big TODO/FIXME
> comment.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
Thank you for taking care of this FIXME.
> t6426: fix TODO about making test more comprehensive
>
> See
> https://lore.kernel.org/git/CABPp-BFxK7SGs3wsOfozSw_Uvr-ynr+x8ciPV2Rmfx6Nr4si6g@mail.gmail.com/
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1462%2Fnewren%2Ft6426-fix-todo-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1462/newren/t6426-fix-todo-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1462
>
> t/t6426-merge-skip-unneeded-updates.sh | 56 ++++++++++----------------
> 1 file changed, 22 insertions(+), 34 deletions(-)
>
> diff --git a/t/t6426-merge-skip-unneeded-updates.sh b/t/t6426-merge-skip-unneeded-updates.sh
> index 2bb8e7f09bb..1fcf5d034ed 100755
> --- a/t/t6426-merge-skip-unneeded-updates.sh
> +++ b/t/t6426-merge-skip-unneeded-updates.sh
> @@ -380,40 +380,28 @@ test_expect_success '2c: Modify b & add c VS rename b->c' '
>
> # Make sure c WAS updated
> test-tool chmtime --get c >new-mtime &&
> - test $(cat old-mtime) -lt $(cat new-mtime)
> -
> - # FIXME: rename/add conflicts are horribly broken right now;
> - # when I get back to my patch series fixing it and
> - # rename/rename(2to1) conflicts to bring them in line with
> - # how add/add conflicts behave, then checks like the below
> - # could be added. But that patch series is waiting until
> - # the rename-directory-detection series lands, which this
> - # is part of. And in the mean time, I do not want to further
> - # enforce broken behavior. So for now, the main test is the
> - # one above that err is an empty file.
> -
> - #git ls-files -s >index_files &&
> - #test_line_count = 2 index_files &&
> -
> - #git rev-parse >actual :2:c :3:c &&
> - #git rev-parse >expect A:b A:c &&
> - #test_cmp expect actual &&
> -
> - #git cat-file -p A:b >>merged &&
> - #git cat-file -p A:c >>merge-me &&
> - #>empty &&
> - #test_must_fail git merge-file \
> - # -L "Temporary merge branch 1" \
> - # -L "" \
> - # -L "Temporary merge branch 2" \
> - # merged empty merge-me &&
> - #sed -e "s/^\([<=>]\)/\1\1\1/" merged >merged-internal &&
> -
> - #git hash-object c >actual &&
> - #git hash-object merged-internal >expect &&
> - #test_cmp expect actual &&
> -
> - #test_path_is_missing b
> + test $(cat old-mtime) -lt $(cat new-mtime) &&
> +
> + git ls-files -s >index_files &&
> + test_line_count = 2 index_files &&
> +
> + git rev-parse >actual :2:c :3:c &&
> + git rev-parse >expect A:c A:b &&
> + test_cmp expect actual &&
> +
> + git cat-file -p A:b >>merge-me &&
> + git cat-file -p A:c >>merged &&
> + >empty &&
> + test_must_fail git merge-file \
> + -L "HEAD" \
> + -L "" \
> + -L "B^0" \
> + merged empty merge-me &&
> + sed -e "s/^\([<=>]\)/\1\1\1/" merged >merged-internal &&
It seems that this line can be dropped, because merged-internal is not
inspected afterwards. None of the other tests in the file do similar
calls to `sed`. Such substitutions with sed are present in
t6422-merge-rename-corner-cases.sh and t6406-merge-attr.sh though.
> +
> + test_cmp merged c &&
> +
> + test_path_is_missing b
Function test_setup_2c() creates commits in order: commit O (create b),
commit A (modify b, create c), and then commit B (rename b->c).
I would have preferred if "test_path_is_missing b" check was done
several lines higher, just before "test_line_count = 2 index_files".
It feels more natural with this order of commits in setup to check what
happened to file "b" first. It would also mean that all checking of
directory contents is done in one place, before merge conflict in file
"c" is inspected.
I see, however, that all tests in this file follow the pattern of
checking missing files at the very end, and consistency might be
preferable here.
> )
> '
>
>
> base-commit: 2b4f5a4e4bb102ac8d967cea653ed753b608193c
Aside from unnecessary call to sed and nitpicking about order of
assertions, the patch looks good to me.
^ permalink raw reply
* ctrl-z ignored by git; creates blobs from non-existent repos
From: Crls @ 2023-01-13 22:01 UTC (permalink / raw)
To: git
Ctrl-Z is ignored by git; Git-clone injects blobs even with non-existent
repos
Steps to reproduce 1- git clone github
whateverrepo/whatevernonexistentrepo or 1- git clone gitlab
whateverrepo/whatevernonexistentrepo 2= Git prompts for a username
3- Press Ctrl-Z to stop *git* from running either on the virtual
console/tty *git* automatically creates blobs with directories and
disregards
^Z altogether It goes ahead anyway and creates pleases. It looks as if
^Z is not enough.
Expected: The same issue does not happen with other non-existent repos
e.g., git clone git.zx2c4/ it returns the message of fatal repo not found
According to console_codes (4) CAN (0x18, ^X) and SUB (0x1A, ^Z)
abort escape sequences should be clear enough
git version 2.36.3
^ permalink raw reply
* Re: [PATCH v5 06/10] test-http-server: add simple authentication
From: Junio C Hamano @ 2023-01-13 21:06 UTC (permalink / raw)
To: Victoria Dye
Cc: Matthew John Cheetham via GitGitGadget, git, Derrick Stolee,
Lessley Dennington, Matthew John Cheetham, M Hickford,
Jeff Hostetler, Glen Choo, Matthew John Cheetham
In-Reply-To: <2a0b5f3f-7ab2-bc9e-76ac-93a52b4d32d0@github.com>
Victoria Dye <vdye@github.com> writes:
>> +static int split_auth_param(const char *str, char **scheme, char **val, int required_val)
>> +{
>> ...
>> +}
>
> There's nothing really *new* in these functions in this iteration, just code
> moved from the option parsing/handling in 'cmd_main()' into dedicated
> functions. Looks good!
> ...
>
> I completely missed the "fall-through" comment in my last review [1], as you
> kindly pointed out [2]. ;) Given that, this makes sense to me.
>> + *wr = send_http_error(STDOUT_FILENO, 401, "Unauthorized", -1,
>> + &hdrs, *wr);
>
> The "extra_headers" configuration is new, and helps make the test server
> more flexible.
This is not limited to this single review message, but it is good to
see "this part of the patch is good because ..." explicitly stated.
I wish more people did so, in addition to pointing out what needs to
be improved.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 0/9] sequencer API & users: fix widespread leaks
From: Junio C Hamano @ 2023-01-13 20:47 UTC (permalink / raw)
To: Phillip Wood
Cc: Ævar Arnfjörð Bjarmason, git, Johannes Schindelin,
Taylor Blau, René Scharfe
In-Reply-To: <e1722673-b444-2dcf-4087-46d2d15b9331@dunelm.org.uk>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Hi Ævar
>
> The code changes in this version look good modulo a couple of minor
> comments, however some of the commit messages need to be updated to
> reflect the (very welcome) changes you've made in v2.
>
> Thanks again for working on this
Thanks, both. Perhaps third-time a charm, with a bit more care on
the presentation ;-)
^ permalink raw reply
* Re: [PATCH] rebase -i: allow a comment after a "break" command
From: Sergey Organov @ 2023-01-13 20:29 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Phillip Wood via GitGitGadget, git, Olliver Schinagl,
Johannes Schindelin, Phillip Wood
In-Reply-To: <xmqqilha18yd.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> I had somewhat the opposite thought. The "break" command is special in
>> that it is not doing anything useful except returning control to the
>> user. And hence producing a message is a useful add-on. So I expected
>> the patch to just allow:
>>
>> break this is a message the user will see
>>
>> without any "#" at all.
>
> Ah, I am OK with that, too.
Then how about this:
break this is a message the user will see # and this they won't
I'm definitely with Elijah in favor of consistent # usage for comments
that go nowhere.
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH] rebase -i: allow a comment after a "break" command
From: Junio C Hamano @ 2023-01-13 20:22 UTC (permalink / raw)
To: Jeff King
Cc: Phillip Wood via GitGitGadget, git, Olliver Schinagl,
Johannes Schindelin, Phillip Wood
In-Reply-To: <Y8A5X0kHE31kSH3z@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I had somewhat the opposite thought. The "break" command is special in
> that it is not doing anything useful except returning control to the
> user. And hence producing a message is a useful add-on. So I expected
> the patch to just allow:
>
> break this is a message the user will see
>
> without any "#" at all.
Ah, I am OK with that, too.
> That does close the door for further arguments, but I have trouble
> imagining what they would be.
Making almost everything that the tool does not pay attention to
(like the patch title of "pick") into comments, floated by Elijah in
the thread, does sound another reasonable direction to go. If we
are not doing "pick 0f2d4b69 # the title of the commit", adding the
message without "#" to "break" might be a better way to go for
consistency.
^ permalink raw reply
* Re: [PATCH] env-helper: move this built-in to to "test-tool env-helper"
From: Junio C Hamano @ 2023-01-13 20:17 UTC (permalink / raw)
To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, git
In-Reply-To: <Y8A3yGeJl0TCDNqe@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Yeah, I am totally fine with this direction. My reservations were:
>
> 1. It was work somebody had to do. But now you've done it.
>
> 2. It's _possible_ that some script somewhere is depending on it. But
> I think the "--" in the name plus the lack of documentation means
> that it's unlikely, and that we're morally absolved if somebody's
> script does break.
That's how we burned some folks who depend on submodule--helper,
isn't it? ;-)
> The patch itself looks obviously correct to me.
>> - test_must_fail git env--helper --type=bool --default=false --exit-code MISSING >actual.out 2>actual.err &&
>> + test_must_fail test-tool env-helper --type=bool --default=false --exit-code MISSING >actual.out 2>actual.err &&
>
> Long lines like these made me wonder if it should simply be "test-tool
> env", which is shorter. We do not need "helper" to avoid polluting the
> main git-command namespace, and everything in test-tool is a helper
> anyway. But it probably doesn't matter much either way. It's not like
> that line wasn't already overly long. :)
I agree that the "-helper" looks a bit irritating, not because the
line is long, but because test-tool is by definition about helpers
used in tests, so the suffix is redundant.
Let's have a small update before queuing it.
> If we do take this, then my t/interop patch can be dropped, though we
> might want to salvage the error message bit:
Yup, that does make sense. Will queue what is below the scissors
line.
Thanks, both.
>
> -- >8 --
> Subject: [PATCH] t/interop: report which vanilla git command failed
>
> The interop test library sets up wrappers "git.a" and "git.b" to
> represent the two versions to be tested. It also wraps vanilla "git" to
> report an error, with the goal of catching tests which accidentally fail
> to use one of the version-specific wrappers (which could invalidate the
> tests in a very subtle way).
>
> But when it catches an invocation of vanilla git, it doesn't give any
> details, which makes it very hard to debug exactly which invocation is
> responsible (especially if it's buried in a function invocation, etc).
> Let's report the arguments passed to git, which helps narrow it down.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/interop/interop-lib.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh
> index 3e0a2911d4..62f4481b6e 100644
> --- a/t/interop/interop-lib.sh
> +++ b/t/interop/interop-lib.sh
> @@ -68,7 +68,7 @@ generate_wrappers () {
> wrap_git .bin/git.a "$DIR_A" &&
> wrap_git .bin/git.b "$DIR_B" &&
> write_script .bin/git <<-\EOF &&
> - echo >&2 fatal: test tried to run generic git
> + echo >&2 fatal: test tried to run generic git: $*
> exit 1
> EOF
> PATH=$(pwd)/.bin:$PATH
^ permalink raw reply
* Re: [PATCH] rebase -i: allow a comment after a "break" command
From: Junio C Hamano @ 2023-01-13 20:17 UTC (permalink / raw)
To: Elijah Newren
Cc: Ævar Arnfjörð Bjarmason,
Phillip Wood via GitGitGadget, git, Olliver Schinagl,
Johannes Schindelin, Phillip Wood, Christian Couder
In-Reply-To: <CABPp-BFf4pbRAy+Oaghx5d8DZgBjY_OUM-rJZna+JyNwx9WB-Q@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> Of course, the same applies to edit/squash/fixup/reword, though if I
> could go back in time...(warning, long tangent coming)...I would make
> it so those four directives did not accept any commit ID argument.
> Only "pick" and "reset" would accept a commit ID. Instead, today's
> "edit X" would be two commands ("pick X" followed by either "break" or
> "edit"), "fixup X" would be "pick X" + "fixup", and "reword X" would
> be "pick X" + "reword". That'd help users understand rebase state
> much better (it's so easy for users to get confused by whether they
> should be using `git commit --amend` vs. `git rebase --continue` and I
> think this is partially to blame, though there's other changes we
> could make to help with that dichotomy as well). The separate
> directives would also make it much easier to figure out how to both
> fixup and edit a single commit in the same rebase (pick the commit,
> then add a fixup directive, then an edit directive).
Intriguing, and I feel sad that it probably is too late for all of
the above X-<.
> In fact, "squash
> X" could just be discarded as superfluous, since it's just "pick X" +
> "fixup" + "reword" (or we could keep squash as an abbreviation for
> both "fixup" + "reword").
IIUC, your "fixup" is
git reset --soft HEAD^
git commit --amend --no-edit
i.e. discard the log message from "fixup" and use only its tree, and
your "reword" is
git commit --amend --edit
so "pick X" + "fixup" + "reword" would not be quite usable as a
replacement of our "squash X" (or your "pick X" + "squash"), I am
afraid. You'd want the log message from "X" as well as "X^" to
edit the replacement of X^.
^ permalink raw reply
* Re: [PATCH] rebase: cleanup "--exec" option handling
From: Andrei Rybak @ 2023-01-13 20:06 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget, git; +Cc: Johannes Schindelin, Phillip Wood
In-Reply-To: <pull.1461.git.1673542201452.gitgitgadget@gmail.com>
On 12/01/2023 17:50, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When handling "--exec" rebase collects the commands into a struct
> string_list, then prepends "exec " to each command creating a multi line
> string and finally splits that string back into a list of commands. This
> is an artifact of the scripted rebase and the need to support "rebase
> --preserve-merges". Now that "--preserve-merges" no-longer exists we can
> cleanup the way the argument is handled. There is no need to add the
> "exec " prefix to the commands as that is added by todo_list_to_strbuf().
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> rebase: cleanup "--exec" option handling
>
> A small cleanup following the removal of "--preserve-merges"
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1461%2Fphillipwood%2Frebase-cleanup-exec-handling-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1461/phillipwood/rebase-cleanup-exec-handling-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1461
>
> builtin/rebase.c | 45 +++++++++++----------------------------------
> sequencer.c | 4 ++--
> 2 files changed, 13 insertions(+), 36 deletions(-)
>
I can't really comment on code, but I appreciate any simplifications
around `rebase --exec`, because it will probably make it easier for
me to revive the topic about `rebase --edit-todo --exec` from the
end of 2019:
https://lore.kernel.org/git/20191114163549.7648-1-rybak.a.v@gmail.com/T/#u
It's current state (experimented a bit a week ago) is on GitHub:
https://github.com/rybak/git/tree/edit-todo-exec
Thank you.
^ permalink raw reply
* Re: [PATCH] ls-tree: add --sparse-filter-oid argument
From: Junio C Hamano @ 2023-01-13 20:01 UTC (permalink / raw)
To: Eric Sunshine; +Cc: William Sprent via GitGitGadget, git, William Sprent
In-Reply-To: <CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
>> Note that one of the tests only pass when run on top of commit
>> 5842710dc2 (dir: check for single file cone patterns, 2023-01-03), which
>> was submitted separately and is currently is merged to 'next'.
>
> Thanks for mentioning this. It's exactly the sort of information the
> maintainer needs when applying your patch to his tree. And it can be
> helpful for reviewers too.
Indeed it is. Thanks for pointing it out---I just wish other
contributors see and follow such a wise piece of advice buried in
review of a patch by others.
^ permalink raw reply
* Re: [PATCH v3 13/19] builtin/merge.c: always free "struct strbuf msg"
From: Junio C Hamano @ 2023-01-13 19:57 UTC (permalink / raw)
To: René Scharfe
Cc: Ævar Arnfjörð Bjarmason, git, Eric Sunshine
In-Reply-To: <71e7d424-b785-9f9e-8b09-955b56fa19b0@web.de>
René Scharfe <l.s.r@web.de> writes:
> Am 10.01.23 um 06:43 schrieb Ævar Arnfjörð Bjarmason:
>> Follow-up 465028e0e25 (merge: add missing strbuf_release(),
>> 2021-10-07) and address the "msg" memory leak in this block. We could
>> free "&msg" before the "goto done" here, but even better is to avoid
>> allocating it in the first place.
>
> So contrary to the subject we don't need to free it anymore.
;-) I appreciate such careful reading.
^ permalink raw reply
* Re: [PATCH v3] scalar: show progress if stderr refer to a terminal
From: Junio C Hamano @ 2023-01-13 19:52 UTC (permalink / raw)
To: Derrick Stolee
Cc: ZheNing Hu via GitGitGadget, git, Johannes Schindelin,
Victoria Dye, Taylor Blau, ZheNing Hu
In-Reply-To: <9d8e38fd-f001-5aa5-ab78-cc6d314df09a@github.com>
Derrick Stolee <derrickstolee@github.com> writes:
> On 1/11/2023 8:14 AM, ZheNing Hu via GitGitGadget wrote:
>> From: ZheNing Hu <adlternative@gmail.com>
>
>> Range-diff vs v2:
>
>> -+test_expect_success 'progress without tty' '
>> ++test_expect_success TTY 'progress without tty' '
>
> I think this addition of the TTY prerequisite is not necessary...
>
>> +test_expect_success TTY 'progress without tty' '
>> + enlistment=progress2 &&
>> +
>> + test_config -C to-clone uploadpack.allowfilter true &&
>> + test_config -C to-clone uploadpack.allowanysha1inwant true &&
>> +
>> + GIT_PROGRESS_DELAY=0 scalar clone "file://$(pwd)/to-clone" "$enlistment" 2>stderr &&
>> + ! grep "Enumerating objects" stderr &&
>> + ! grep "Updating files" stderr &&
>> + cleanup_clone $enlistment
>> +'
>
> ...because the test doesn't use the environment details for
> mimicing a TTY. The point is that stderr is redirected to a
> file and isatty(2) would report false.
Yup, the prerequisite was uttering misleading. I may queue it with
local tweaks, but if I forget please send in an update.
Thanks.
> I don't think this is worth a re-roll, though, so I'm happy
> with this version.
>
> Thanks,
> -Stolee
^ permalink raw reply
* Re: [PATCH v2] date.c: allow ISO 8601 reduced precision times
From: Junio C Hamano @ 2023-01-13 19:50 UTC (permalink / raw)
To: Đoàn Trần Công Danh; +Cc: git, Phil Hord, plavarre
In-Reply-To: <20230111001003.10916-1-congdanhqx@gmail.com>
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes:
> ISO 8601 permits "reduced precision" time representations to omit the
> seconds value or both the minutes and the seconds values. The
> abbreviate times could look like 17:45 or 1745 to omit the seconds,
> or simply as 17 to omit both the minutes and the seconds.
>
> parse_date_basic accepts the 17:45 format but it rejects the other two.
> Change it to accept 4-digit and 2-digit time values when they follow a
> recognized date and a 'T'.
>
> Before this change:
>
> $ TZ=UTC test-tool date approxidate 2022-12-13T23:00 2022-12-13T2300 2022-12-13T23
> 2022-12-13T23:00 -> 2022-12-13 23:00:00 +0000
> 2022-12-13T2300 -> 2022-12-13 23:54:13 +0000
> 2022-12-13T23 -> 2022-12-13 23:54:13 +0000
>
> After this change:
>
> $ TZ=UTC helper/test-tool date approxidate 2022-12-13T23:00 2022-12-13T2300 2022-12-13T23
> 2022-12-13T23:00 -> 2022-12-13 23:00:00 +0000
> 2022-12-13T2300 -> 2022-12-13 23:00:00 +0000
> 2022-12-13T23 -> 2022-12-13 23:00:00 +0000
>
> Note: ISO 8601 also allows reduced precision date strings such as
> "2022-12" and "2022". This patch does not attempt to address these.
>
> Reported-by: Pat LaVarre <plavarre@purestorage.com>
> Signed-off-by: Phil Hord <phil.hord@gmail.com>
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>
> Since this is a complete re-implementation from Phil Hord's version.
> I'm reassigning the author to me.
>
> This version change the implementation to only treat the string as ISO8601 if
> a 'T' existed and date has been parsed. I also added a test for parsing
> RFC-822, which Hord accidentally broke.
>
> The commit message has been changed:
> * The example has been changed to be independent from local timezone
> * Remove the mention of adding test-cases, since it's obviously necessary.
Will replace Phil's patch with this (hence even under your
authorship, the topic name will be reused).
Thanks, both.
^ permalink raw reply
* Re: [PATCH v2 0/6] cache API: always have a "istate->repo"
From: Junio C Hamano @ 2023-01-13 19:22 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Derrick Stolee, Victoria Dye, Jeff Hostetler
In-Reply-To: <cover-v2-0.6-00000000000-20230112T124842Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> The "struct index_state" contains a "repo" member, which should be a
> pointer to the repository for that index, but which due to us
> constructing such structs on an ad-hoc basis in various places wasn't
> always available.
I'd exclude 6/6 for now, as it seems to depend on some changes only
in 'next'. Feel free to resend only that step with reduced scope so
that it applies to 'master', and send in incremental updates when
each of these topics that are only in 'next' graduates.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 0/6] cache API: always have a "istate->repo"
From: Junio C Hamano @ 2023-01-13 18:29 UTC (permalink / raw)
To: Derrick Stolee
Cc: Ævar Arnfjörð Bjarmason, git, Victoria Dye,
Jeff Hostetler
In-Reply-To: <26708e4a-93bf-5dec-2b3e-da8f4ce37571@github.com>
Derrick Stolee <derrickstolee@github.com> writes:
> I wasn't expecting the initializer idea to work as well as it has.
> Now, Patch 5 does all the heavy lifting and Patch 6 is an easy read.
>
> Outside of one question about the istate->initialized member (which
> might not need any change) I'm happy with this version.
Thanks, both. And especially for finding 913e0e99 (unpack_trees():
protect the handcrafted in-core index from read_cache(),
2008-08-23).
Will queue.
^ permalink raw reply
* Re: [PATCH v2] merge: break out of all_strategy loop when strategy is found
From: Junio C Hamano @ 2023-01-13 18:24 UTC (permalink / raw)
To: Rose via GitGitGadget; +Cc: git, Seija Kijin
In-Reply-To: <pull.1429.v2.git.git.1673285669004.gitgitgadget@gmail.com>
"Rose via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Seija Kijin <doremylover123@gmail.com>
>
> strncmp does not modify any of the memory.
It may be a correct statement, but so what? It does not seem to
have relevance to this change.
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 0f093f2a4f2..74de2ebd2b3 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -188,7 +188,7 @@ static struct strategy *get_strategy(const char *name)
> for (i = 0; i < main_cmds.cnt; i++) {
> int j, found = 0;
> struct cmdname *ent = main_cmds.names[i];
> - for (j = 0; j < ARRAY_SIZE(all_strategy); j++)
> + for (j = 0; !found && j < ARRAY_SIZE(all_strategy); j++)
> if (!strncmp(ent->name, all_strategy[j].name, ent->len)
> && !all_strategy[j].name[ent->len])
> found = 1;
I am not sure if this micro-optimization is worth it.
If this loop is so costly that it needs optimization, a better thing
to do would be to rethink the way it filters main_cmds.names[] array
with all_strategy[]. The latter is a fairly small, and more
importantly, a constant set of known strategies, so there should be
a more efficient way than O(n*m) nested loop.
The code churn has already costed us too much, mostly reviewer and
maintainer time, for the value of the change itself. I'll queue the
patch as-is, because this change is not making anything worse
per-se, but primarily because I do not want the topic to take any
more of our resources.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 06/10] test-http-server: add simple authentication
From: Victoria Dye @ 2023-01-13 18:10 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget, git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Matthew John Cheetham
In-Reply-To: <c3c3d17a688963acc180e3bb7bbb4deb32a94304.1673475190.git.gitgitgadget@gmail.com>
Matthew John Cheetham via GitGitGadget wrote:
> +static struct auth_module *create_auth_module(const char *scheme,
> + const char *challenge)
> +{
> + struct auth_module *mod = xmalloc(sizeof(struct auth_module));
> + mod->scheme = xstrdup(scheme);
> + mod->challenge_params = challenge ? xstrdup(challenge) : NULL;
> + CALLOC_ARRAY(mod->tokens, 1);
> + string_list_init_dup(mod->tokens);
> + return mod;
> +}
> +
> +static int add_auth_module(struct auth_module *mod)
> +{
> + if (get_auth_module(mod->scheme))
> + return error("duplicate auth scheme '%s'\n", mod->scheme);
> +
> + ALLOC_GROW(auth_modules, auth_modules_nr + 1, auth_modules_alloc);
> + auth_modules[auth_modules_nr++] = mod;
> +
> + return 0;
> +}
> +static int split_auth_param(const char *str, char **scheme, char **val, int required_val)
> +{
> + struct strbuf **p = strbuf_split_str(str, ':', 2);
> +
> + if (!p[0])
> + return -1;
> +
> + /* trim trailing ':' */
> + if (p[1])
> + strbuf_setlen(p[0], p[0]->len - 1);
> +
> + if (required_val && !p[1])
> + return -1;
> +
> + *scheme = strbuf_detach(p[0], NULL);
> +
> + if (p[1])
> + *val = strbuf_detach(p[1], NULL);
> +
> + strbuf_list_free(p);
> + return 0;
> +}
There's nothing really *new* in these functions in this iteration, just code
moved from the option parsing/handling in 'cmd_main()' into dedicated
functions. Looks good!
> + switch (result) {
> + case AUTH_ALLOW:
> + trace2_printf("%s: auth '%s' ALLOW", TR2_CAT, mod->scheme);
> + *user = "VALID_TEST_USER";
> + *wr = WR_OK;
> + break;
> +
> + case AUTH_DENY:
> + trace2_printf("%s: auth '%s' DENY", TR2_CAT, mod->scheme);
> + /* fall-through */
> +
> + case AUTH_UNKNOWN:
> + if (result != AUTH_DENY && allow_anonymous)
> + break;
I completely missed the "fall-through" comment in my last review [1], as you
kindly pointed out [2]. ;) Given that, this makes sense to me.
[1] https://lore.kernel.org/git/2a5d6586-3d2c-8af4-12be-a5a106f966b5@github.com/
[2] https://lore.kernel.org/git/AS2PR03MB981593EB3382F9738D2CA3D7C0FC9@AS2PR03MB9815.eurprd03.prod.outlook.com/
> +
> + for (i = 0; i < auth_modules_nr; i++) {
> + mod = auth_modules[i];
> + if (mod->challenge_params)
> + challenge = xstrfmt("WWW-Authenticate: %s %s",
> + mod->scheme,
> + mod->challenge_params);
> + else
> + challenge = xstrfmt("WWW-Authenticate: %s",
> + mod->scheme);
> + string_list_append(&hdrs, challenge);
> + }
> +
> + for (i = 0; i < extra_headers.nr; i++)
> + string_list_append(&hdrs, extra_headers.v[i]);
> +
> + *wr = send_http_error(STDOUT_FILENO, 401, "Unauthorized", -1,
> + &hdrs, *wr);
The "extra_headers" configuration is new, and helps make the test server
more flexible.
> +static int read_auth_config(const char *name, const char *val, void *data)
> +{
> + int ret = 0;
> + char *scheme = NULL;
> + char *token = NULL;
> + char *challenge = NULL;
> + struct auth_module *mod = NULL;
> +
> + if (!strcmp(name, "auth.challenge")) {
> + if (split_auth_param(val, &scheme, &challenge, 0)) {
> + ret = error("invalid auth challenge '%s'", val);
> + goto cleanup;
> + }
> +
> + mod = create_auth_module(scheme, challenge);
> + if (add_auth_module(mod)) {
> + ret = error("failed to add auth module '%s'", val);
> + goto cleanup;
> + }
> + }
> + if (!strcmp(name, "auth.token")) {
> + if (split_auth_param(val, &scheme, &token, 1)) {
> + ret = error("invalid auth token '%s'", val);
> + goto cleanup;
> + }
> +
> + mod = get_auth_module(scheme);
> + if (!mod) {
> + ret = error("auth scheme not defined '%s'\n", scheme);
> + goto cleanup;
> + }
> +
> + string_list_append(mod->tokens, token);
> + }
I don't think this addresses the implicit option ordering requirement noted
in [3]; instead of needing '--auth' before '--auth-token', this now needs
'auth.challenge' before 'auth.token' in the config file. While I'd prefer it
if this could be rearranged so that the auth setup happens after all config
parsing (so the order doesn't matter), if you want to leave it as-is please
add a comment somewhere in this file explaining that requirement and/or add
a note to the "auth scheme not defined" error message.
[3] https://lore.kernel.org/git/2a5d6586-3d2c-8af4-12be-a5a106f966b5@github.com/
> + if (!strcmp(name, "auth.allowanonymous")) {
> + allow_anonymous = git_config_bool(name, val);
> + }
> + if (!strcmp(name, "auth.extraheader")) {
> + strvec_push(&extra_headers, val);
> + }
Is it worth printing a warning if the option found isn't any of the above?
Something like "ignoring <config option>". This is a test helper, so
user-friendliness isn't quite as important as it is for builtins, but the
warning might be helpful to developers trying to use it in the future.
> +
> +cleanup:
> + free(scheme);
> + free(token);
> + free(challenge);
> +
> + return ret;
> +}
> +
> static enum worker_result dispatch(struct req *req)
> {
> + enum worker_result wr = WR_OK;
> + const char *user = NULL;
> +
> + if (!is_authed(req, &user, &wr))
> + return wr;
> +
> if (is_git_request(req))
> - return do__git(req);
> + return do__git(req, user);
>
> return send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1, NULL,
> WR_OK | WR_HANGUP);
> @@ -624,6 +853,19 @@ int cmd_main(int argc, const char **argv)
> pid_file = v;
> continue;
> }
> + if (skip_prefix(arg, "--auth-config=", &v)) {
> + if (!strlen(v)) {
> + error("invalid argument - missing file path");
> + usage(test_http_auth_usage);
> + }
> +
> + if (git_config_from_file(read_auth_config, v, NULL)) {
> + error("failed to read auth config file '%s'", v);
> + usage(test_http_auth_usage);
> + }
> +
> + continue;
> + }
>
> fprintf(stderr, "error: unknown argument '%s'\n", arg);
> usage(test_http_auth_usage);
^ permalink raw reply
* Re: [PATCH v2] doc: add "git switch -c" as another option on detached HEAD
From: Junio C Hamano @ 2023-01-13 17:58 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Yutaro Ohno via GitGitGadget, git, Yutaro Ohno
In-Reply-To: <CAPig+cTHMEeUw8PVg7nWU8+jdo=yN9nC+RRuaSiVg8D3Or-nNQ@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> Thanks. This version looks good to me and addresses reviewer comments[1,2,3].
Thanks, both. Let's queue and merge it to 'next'.
^ permalink raw reply
* Re: [PATCH v2 0/5] Documentation: updates and a correction around 'ORIG_HEAD'
From: Junio C Hamano @ 2023-01-13 17:56 UTC (permalink / raw)
To: Phillip Wood
Cc: Philippe Blain via GitGitGadget, git, Erik Cervin Edin,
Philippe Blain
In-Reply-To: <ba20039c-19a3-96da-c938-89af67bca791@dunelm.org.uk>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Hi Philippe
>
> On 10/01/2023 13:15, Philippe Blain via GitGitGadget wrote:
>> * added a link to the mailing list thread in the commit message of 5/5.
>> v1: Documentation: updates and a correction around 'ORIG_HEAD'
>> This series' initial motivation was to clear up a confusion that
>> arose in
>> [1] where it was noticed that 'ORIG_HEAD' is not guaranteed to point to the
>> original branch tip at the end of the rebase if 'git reset' is used during
>> the rebase.
>> Patch 5/5 adds a note to 'git rebase's documentation to make that
>> explicit.
>> When taking a look at the existing documentation mentioning 'ORIG_HEAD', I
>> also found an error in an example (patch 1/5), other small inconsistencies
>> (patch 2-3/5), and a potential improvement (patch 4/5).
>
> Thanks for doing this, I think the adding a note to the rebase
> documentation is a good idea given the confusion that's been
> reported. The patches all look great to me apart from patch 4 where I
> share Junio's concerns about the readability and maintenance burden of
> the list of commands. I was surprised to learn that merge also sets
> ORIG_HEAD, thanks for being so thorough.
Thanks for reviewing, and thanks for writing, the patch. Let's
queue and merge it to 'next'.
^ 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