* [PATCH v5 1/2] send-email: refactor header generation functions
From: Strawbridge, Michael @ 2023-01-17 1:37 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Strawbridge, Michael, Tuikov, Luben, Junio C Hamano
In-Reply-To: <20230117013709.47054-1-michael.strawbridge@amd.com>
Split process_file and send_message into easier to use functions.
Making SMTP header information more widely available.
Cc: Luben Tuikov <luben.tuikov@amd.com>
Cc: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
git-send-email.perl | 49 ++++++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 18 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 5861e99a6e..810dd1f1ce 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1495,16 +1495,7 @@ sub file_name_is_absolute {
return File::Spec::Functions::file_name_is_absolute($path);
}
-# Prepares the email, then asks the user what to do.
-#
-# If the user chooses to send the email, it's sent and 1 is returned.
-# If the user chooses not to send the email, 0 is returned.
-# If the user decides they want to make further edits, -1 is returned and the
-# caller is expected to call send_message again after the edits are performed.
-#
-# If an error occurs sending the email, this just dies.
-
-sub send_message {
+sub gen_header {
my @recipients = unique_email_list(@to);
@cc = (grep { my $cc = extract_valid_address_or_die($_);
not grep { $cc eq $_ || $_ =~ /<\Q${cc}\E>$/ } @recipients
@@ -1546,6 +1537,22 @@ sub send_message {
if (@xh) {
$header .= join("\n", @xh) . "\n";
}
+ my $recipients_ref = \@recipients;
+ return ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header);
+}
+
+# Prepares the email, then asks the user what to do.
+#
+# If the user chooses to send the email, it's sent and 1 is returned.
+# If the user chooses not to send the email, 0 is returned.
+# If the user decides they want to make further edits, -1 is returned and the
+# caller is expected to call send_message again after the edits are performed.
+#
+# If an error occurs sending the email, this just dies.
+
+sub send_message {
+ my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
+ my @recipients = @$recipients_ref;
my @sendmail_parameters = ('-i', @recipients);
my $raw_from = $sender;
@@ -1735,11 +1742,8 @@ sub send_message {
$references = $initial_in_reply_to || '';
$message_num = 0;
-# Prepares the email, prompts the user, sends it out
-# Returns 0 if an edit was done and the function should be called again, or 1
-# otherwise.
-sub process_file {
- my ($t) = @_;
+sub pre_process_file {
+ my ($t, $quiet) = @_;
open my $fh, "<", $t or die sprintf(__("can't open file %s"), $t);
@@ -1893,9 +1897,9 @@ sub process_file {
}
close $fh;
- push @to, recipients_cmd("to-cmd", "to", $to_cmd, $t)
+ push @to, recipients_cmd("to-cmd", "to", $to_cmd, $t, $quiet)
if defined $to_cmd;
- push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t)
+ push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t, $quiet)
if defined $cc_cmd && !$suppress_cc{'cccmd'};
if ($broken_encoding{$t} && !$has_content_type) {
@@ -1954,6 +1958,15 @@ sub process_file {
@initial_to = @to;
}
}
+}
+
+# Prepares the email, prompts the user, sends it out
+# Returns 0 if an edit was done and the function should be called again, or 1
+# otherwise.
+sub process_file {
+ my ($t) = @_;
+
+ pre_process_file($t, $quiet);
my $message_was_sent = send_message();
if ($message_was_sent == -1) {
@@ -2002,7 +2015,7 @@ sub process_file {
# Execute a command (e.g. $to_cmd) to get a list of email addresses
# and return a results array
sub recipients_cmd {
- my ($prefix, $what, $cmd, $file) = @_;
+ my ($prefix, $what, $cmd, $file, $quiet) = @_;
my @addresses = ();
open my $fh, "-|", "$cmd \Q$file\E"
--
2.34.1
^ permalink raw reply related
* [PATCH v5 0/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Strawbridge, Michael @ 2023-01-17 1:37 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Strawbridge, Michael
Hi Junio,
I very much appreciate the feedback and believe I have changed things to match.
To answer your earlier question, the hook doesn't need to support multiple header capitalizations (ie. only Cc is passed). 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.
Michael Strawbridge (2):
send-email: refactor header generation functions
send-email: expose header information to git-send-email's
sendemail-validate hook
Documentation/githooks.txt | 17 ++++++--
git-send-email.perl | 80 +++++++++++++++++++++++++-------------
t/t9001-send-email.sh | 29 +++++++++++++-
3 files changed, 92 insertions(+), 34 deletions(-)
--
2.34.1
^ permalink raw reply
* Re: [PATCH] builtin/checkout: check the branch used in -B with worktrees
From: Rubén Justo @ 2023-01-17 0:53 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón, git; +Cc: vustthat, sunshine, pclouds
In-Reply-To: <20230116172824.93218-1-carenas@gmail.com>
Hi Carlo.
Thank you for working on this.
On 16/1/23 18:28, Carlo Marcelo Arenas Belón wrote:
> @@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts,
> if (!opts->can_switch_when_in_progress)
> die_if_some_operation_in_progress();
>
> - if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
> - !opts->ignore_other_worktrees) {
> + if (check_branch_info->path && !opts->force_detach && !opts->ignore_other_worktrees) {
> int flag;
> char *head_ref = resolve_refdup("HEAD", 0, NULL, &flag);
> if (head_ref &&
> - (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))
> - die_if_checked_out(new_branch_info->path, 1);
> + (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path)))
> + die_if_checked_out(check_branch_info->path, 1);
You adjusted this block to accommodate the problem with "checkout -B",
which is sensible, but we may need to do something different here.
What we are doing here, if I understand it correctly, is dying if the
branch is _not checked out in the current worktree_ and _checked out in
any other worktree_. Which is OK for normal checkout/switch.
But IMHO with "checkout -B" we have to die if the branch is checked out
in any other worktree, regardless of whether or not it is checked out in
the current working tree.
Perhaps the scenario where the user has the same branch checked out in
multiple worktrees and tries to reset in one of them, is one of those
where we could use the "if it hurts, don't do it". But we are providing
the "--ignore-other-worktrees" modifier, so I think we should disallow
the "-B" if the branch is checked out in any other worktree, and let
the user use "--ignore-other-worktrees" if he wants to go wild.
But.. die_if_checked_out() does not correctly honor the
"ignore_current_worktree" in this situation. I have submitted a patch
to fix this, in case you want to consider all of this.
Un saludo.
^ permalink raw reply
* [PATCH] worktree: teach find_shared_symref to ignore current worktree
From: Rubén Justo @ 2023-01-17 0:36 UTC (permalink / raw)
To: Git List; +Cc: Eric Sunshine
We prevent some operations from being executed on a branch checked out
in a worktree other than the current one. An example of this was
introduced in b5cabb4 (rebase: refuse to switch to branch already
checked out elsewhere, 2020-02-23).
"find_shared_symref()" is sometimes used to find the worktree in which a
branch is checked out. It performs its search starting with the current
worktree.
As we allow to have the same branch checked out in multiple worktrees
simultaneously...
$ git worktree add foo
$ git worktree add -f bar foo
$ git checkout --ignore-other-worktrees foo
... if the branch checked out in the current worktree is also checked
out in another worktree, with "find_shared_symref()" we will not notice
this "other" working tree.
Let's teach "find_shared_symref()" to ignore the current worktree in the
search, based on the caller's needs.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
branch.c | 4 ++--
builtin/notes.c | 2 +-
builtin/receive-pack.c | 2 +-
t/t3400-rebase.sh | 3 +++
worktree.c | 6 +++++-
worktree.h | 3 ++-
6 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/branch.c b/branch.c
index d182756827..2508e94add 100644
--- a/branch.c
+++ b/branch.c
@@ -822,8 +822,8 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)
struct worktree **worktrees = get_worktrees();
const struct worktree *wt;
- wt = find_shared_symref(worktrees, "HEAD", branch);
- if (wt && (!ignore_current_worktree || !wt->is_current)) {
+ wt = find_shared_symref(worktrees, "HEAD", branch, ignore_current_worktree);
+ if (wt) {
skip_prefix(branch, "refs/heads/", &branch);
die(_("'%s' is already checked out at '%s'"), branch, wt->path);
}
diff --git a/builtin/notes.c b/builtin/notes.c
index 80d9dfd25c..80326bdaab 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -869,7 +869,7 @@ static int merge(int argc, const char **argv, const char *prefix)
/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */
worktrees = get_worktrees();
wt = find_shared_symref(worktrees, "NOTES_MERGE_REF",
- default_notes_ref());
+ default_notes_ref(), 0);
if (wt)
die(_("a notes merge into %s is already in-progress at %s"),
default_notes_ref(), wt->path);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a90af30363..18d400101c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1460,7 +1460,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
int do_update_worktree = 0;
struct worktree **worktrees = get_worktrees();
const struct worktree *worktree =
- find_shared_symref(worktrees, "HEAD", name);
+ find_shared_symref(worktrees, "HEAD", name, 0);
/* only refs/... are allowed */
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index d5a8ee39fc..874cfff8fe 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -407,6 +407,9 @@ test_expect_success 'refuse to switch to branch checked out elsewhere' '
git checkout main &&
git worktree add wt &&
test_must_fail git -C wt rebase main main 2>err &&
+ test_i18ngrep "already checked out" err &&
+ git worktree add --force wt2 main &&
+ test_must_fail git rebase main main &&
test_i18ngrep "already checked out" err
'
diff --git a/worktree.c b/worktree.c
index aa43c64119..3d686137ef 100644
--- a/worktree.c
+++ b/worktree.c
@@ -405,7 +405,8 @@ int is_worktree_being_bisected(const struct worktree *wt,
*/
const struct worktree *find_shared_symref(struct worktree **worktrees,
const char *symref,
- const char *target)
+ const char *target,
+ int ignore_current_worktree)
{
const struct worktree *existing = NULL;
int i = 0;
@@ -416,6 +417,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
struct ref_store *refs;
int flags;
+ if (wt->is_current && ignore_current_worktree)
+ continue;
+
if (wt->is_bare)
continue;
diff --git a/worktree.h b/worktree.h
index 9dcea6fc8c..a9f35ee990 100644
--- a/worktree.h
+++ b/worktree.h
@@ -147,7 +147,8 @@ void free_worktrees(struct worktree **);
*/
const struct worktree *find_shared_symref(struct worktree **worktrees,
const char *symref,
- const char *target);
+ const char *target,
+ int ignore_current_worktree);
/*
* Similar to head_ref() for all HEADs _except_ one from the current
--
2.39.0
^ permalink raw reply related
* Re: [PATCH] builtin/checkout: check the branch used in -B with worktrees
From: Eric Sunshine @ 2023-01-16 22:18 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón; +Cc: git, vustthat, pclouds
In-Reply-To: <20230116172824.93218-1-carenas@gmail.com>
On Mon, Jan 16, 2023 at 12:30 PM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> builtin/checkout: check the branch used in -B with worktrees
Thanks for working on this and coming up with a fix. As mentioned
earlier, I had started looking into it[1], but lacked the time to
disentangle the logic, so I'm glad to see a patch arrive so quickly.
> When multiple worktrees are being used, checkout/switch check
> that the target branch is not already checked out with code that
> evolved from 8d9fdd7087 (worktree.c: check whether branch is rebased
> in another worktree, 2016-04-22), but that logic wasn't valid for
> -B/-C
>
> Avoid reusing the same `branch_info` structure for the checks and
> assumming that it will be rejected later if is a new branch that
> already exists as that doesn't apply to -B/-C.
Even though I'm familiar with the bug report[2] which sparked this
patch, I find the above description somewhat hard to digest; the
high-level problem it is addressing doesn't jump off the page at me.
Perhaps it can be rewritten something like this:
checkout/switch: disallow checking out same branch in multiple worktrees
Commands `git switch -C` and `git checkout -B` neglect to check
whether the branch being forcibly created is already checked out
in some other worktree, which can result in the undesirable
situation of the same branch being checked out in multiple
worktrees. For instance:
$ git worktree list
.../foo beefb00f [main]
$ git worktree add ../other
Preparing worktree (new branch 'other')
HEAD is now at beefb00f first
$ cd ../other
$ git switch -C main
Switched to and reset branch 'main'
$ git worktree list
.../foo beefb00f [main]
.../other beefb00f [main]
$
Fix this problem by teaching `git switch -C` and `git checkout -B`
to check whether the branch in question is already checked out
elsewhere.
after which you might include some details which you wrote about initially.
> Reported-by: Jinwook Jeong <vustthat@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> @@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts,
> - if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
> - !opts->ignore_other_worktrees) {
> + if (check_branch_info->path && !opts->force_detach && !opts->ignore_other_worktrees) {
> int flag;
> char *head_ref = resolve_refdup("HEAD", 0, NULL, &flag);
> if (head_ref &&
> - (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))
> - die_if_checked_out(new_branch_info->path, 1);
> + (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path)))
> + die_if_checked_out(check_branch_info->path, 1);
This variable name change (`new_branch_info` => `check_branch_info`)
helps make the code clearer. Good. (I had found it more than a little
confusing to have similar named variables `new_branch_info` and
`opts->new_branch` even though they are unrelated and have very
different purposes.)
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> @@ -125,6 +125,13 @@ test_expect_success 'die the same branch is already checked out' '
> +test_expect_success 'die the same branch is already checked out (checkout -B)' '
> + (
> + cd here &&
> + test_must_fail git checkout -B newmain
> + )
> +'
Although `git switch` and `git checkout` currently share
implementation, that might not always be the case going forward. As
such, this test could be made a bit more robust by testing both
commands rather than just `git-checkout`. So, perhaps:
test_expect_success 'die the same branch is already checked out' '
(
cd here &&
test_must_fail git checkout -B newmain &&
test_must_fail git switch -C newmain
)
'
> +test_expect_success 'not die on re-checking out current branch (checkout -B)' '
> + (
> + cd there &&
> + git checkout -B newmain
> + )
> +'
Good to see you considered this case too. (I had tested it myself
manually when trying out your patch.)
[1]: https://lore.kernel.org/git/CAPig+cQc1+D9gH7BAC-r03bGKWx3a9jpPyLuP-ehH-X2P+fV6Q@mail.gmail.com/
[2]: https://lore.kernel.org/git/CAA3Q-aaO=vcZd9VLFr8UP-g06be80eUWN_GjygfyGkYmrLx9yQ@mail.gmail.com/
^ permalink raw reply
* Re: [PATCH v2] grep: correctly identify utf-8 characters with \{b,w} in -P
From: Junio C Hamano @ 2023-01-16 20:48 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Carlo Marcelo Arenas Belón, git
In-Reply-To: <230109.86v8lf297g.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> But I don't think it's safe to change the default behavior "git-grep",
> it's not a mere bug fix, but a major behavior change for existing users
> of grep.patternType=perl.
> ...
> Even for Perl, this behavior has been troublesome. Opinions differ, but
> I think many would agree (and I've CC'd the main authority on Perl's
> regex engine) that doing this by default was *probably* a mistake.
> ...
> As the example at the start shows you can already do this with "(*UCP)"
> in the pattern, so perhaps we should just link to the pcre2pattern(3)
> manual from git-grep(1)?
So, now do we have a final verdict on this patch? If we are not
taking the "unconditonally enable ucp" patch (which I tend to agree
with a safer choice for now), it may make sense to mention (*UCP) in
our documentation somewhere, perhaps?
^ permalink raw reply
* Re: [PATCH] format-patch: unleak "-v <num>"
From: Jeff King @ 2023-01-16 19:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqpmbecoom.fsf@gitster.g>
On Mon, Jan 16, 2023 at 10:35:53AM -0800, Junio C Hamano wrote:
> > The word "unleak" in the subject made me think about UNLEAK(), so this
> > is a small tangent. This is exactly the kind of case that I designed
> > UNLEAK() for, because the solution really is "while you are assigning to
> > X, also keep a copy of the pointer in Y to be freed later".
>
> Yup. I was originally planning to use UNLEAK(), but it felt ugly to
> UNLEAK(rev.subject_prefix), as it stores borrowed pointer sometimes
> and owned pointer some other times, which is the exact reason why I
> started looking for a clean way to plug this leak. So I ended up
> with declaring that the member should only store a borrowed pointer.
That's actually one of the nice things about UNLEAK(). It is OK to
over-mark something that may or may not be allocated.
> The first sentence needs to be rephrased, as it does not make much
> sense to have something usually be X and always be X at the same
> time (I'd just remove "always" from there).
Yep, agreed.
-Peff
^ permalink raw reply
* Re: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Michal Suchánek @ 2023-01-16 19:08 UTC (permalink / raw)
To: Hans Petter Selasky; +Cc: git
In-Reply-To: <6a398405-e5f8-0b78-e463-41d79e49e78b@selasky.org>
On Mon, Jan 16, 2023 at 10:55:34AM +0100, Hans Petter Selasky wrote:
> On 1/16/23 10:13, Michal Suchánek wrote:
> > when that data is copied to a new location a new
> > CRC is calculated that can detect an error in that location.
>
> Yes, that is correct, but what is "copying data"? Are you saying that
> copying data is always error free?
Maybe you should not cut out the answer to your qestion?
Thanks
Michal
^ permalink raw reply
* [PATCH v3 1/1] cat-file: quote-format name in error when using -z
From: Toon Claes @ 2023-01-16 19:07 UTC (permalink / raw)
To: git; +Cc: Toon Claes, Phillip Wood
In-Reply-To: <20230116190749.4141516-1-toon@iotcl.com>
Since it's supported to have NUL-delimited input, introduced in
db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,
2022-07-22), it's possible to pass paths that contain newlines. This
works great when the object is found, but when it's not, the input path
is returned in the error message. Because this can contain newlines, the
error message might get spread over multiple lines, making it harder to
machine-parse this error message.
With this change, the input is quote-formatted in the error message, if
needed. This ensures the error message is always on a single line and
makes parsing the error more straightforward.
Signed-off-by: Toon Claes <toon@iotcl.com>
---
builtin/cat-file.c | 19 +++++++++++++++++++
t/t1006-cat-file.sh | 8 ++++++++
2 files changed, 27 insertions(+)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index cc17635e76..b678f69773 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -14,6 +14,7 @@
#include "tree-walk.h"
#include "oid-array.h"
#include "packfile.h"
+#include "quote.h"
#include "object-store.h"
#include "promisor-remote.h"
#include "mailmap.h"
@@ -455,8 +456,17 @@ static void batch_object_write(const char *obj_name,
&data->oid, &data->info,
OBJECT_INFO_LOOKUP_REPLACE);
if (ret < 0) {
+ struct strbuf quoted = STRBUF_INIT;
+
+ if (opt->nul_terminated &&
+ obj_name) {
+ quote_c_style(obj_name, "ed, NULL, 0);
+ obj_name = quoted.buf;
+ }
+
printf("%s missing\n",
obj_name ? obj_name : oid_to_hex(&data->oid));
+ strbuf_release("ed);
fflush(stdout);
return;
}
@@ -503,6 +513,13 @@ static void batch_one_object(const char *obj_name,
result = get_oid_with_context(the_repository, obj_name,
flags, &data->oid, &ctx);
if (result != FOUND) {
+ struct strbuf quoted = STRBUF_INIT;
+
+ if (opt->nul_terminated) {
+ quote_c_style(obj_name, "ed, NULL, 0);
+ obj_name = quoted.buf;
+ }
+
switch (result) {
case MISSING_OBJECT:
printf("%s missing\n", obj_name);
@@ -527,6 +544,8 @@ static void batch_one_object(const char *obj_name,
result);
break;
}
+
+ strbuf_release("ed);
fflush(stdout);
return;
}
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 23b8942edb..e8ce20e739 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -447,6 +447,14 @@ test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '
test_cmp expect actual
'
+test_expect_success '--batch-check, -z with newline in non-existent named object' '
+ printf "HEAD:newline${LF}missing" >in &&
+ git cat-file --batch-check -z <in >actual &&
+
+ printf "\"HEAD:newline\\\\nmissing\" missing\n" >expect &&
+ test_cmp expect actual
+'
+
batch_command_multiple_info="info $hello_sha1
info $tree_sha1
info $commit_sha1
--
2.39.0.rc0.57.g2e71cbbddd.dirty
^ permalink raw reply related
* [PATCH v3 0/1] cat-file: quote-format name in error when using -z
From: Toon Claes @ 2023-01-16 19:07 UTC (permalink / raw)
To: git; +Cc: Toon Claes, Phillip Wood
In-Reply-To: <20230105062447.2943709-1-toon@iotcl.com>
Hi there,
This third revision of this patch fixes broken test case that was added in
t1006.
Toon Claes (1):
cat-file: quote-format name in error when using -z
builtin/cat-file.c | 19 +++++++++++++++++++
t/t1006-cat-file.sh | 8 ++++++++
2 files changed, 27 insertions(+)
Range-diff against v2:
1: 4f39eb0719 ! 1: a56932572c cat-file: quote-format name in error when using -z
@@ t/t1006-cat-file.sh: test_expect_success FUNNYNAMES '--batch-check, -z with newl
'
+test_expect_success '--batch-check, -z with newline in non-existent named object' '
-+ printf "HEAD:newline${LF}embedded" >in &&
++ printf "HEAD:newline${LF}missing" >in &&
+ git cat-file --batch-check -z <in >actual &&
+
-+ printf "\"HEAD:newline\\\\nembedded\" missing\n" >expect &&
++ printf "\"HEAD:newline\\\\nmissing\" missing\n" >expect &&
+ test_cmp expect actual
+'
+
--
2.39.0.rc0.57.g2e71cbbddd.dirty
^ permalink raw reply
* Re: [PATCH v2 0/6] cache API: always have a "istate->repo"
From: Junio C Hamano @ 2023-01-16 18:39 UTC (permalink / raw)
To: Philip Oakley
Cc: Ævar Arnfjörð Bjarmason, git, Derrick Stolee,
Victoria Dye, Jeff Hostetler
In-Reply-To: <b2eca967-4c52-3f8d-f3fe-b1ed41b06280@iee.email>
Philip Oakley <philipoakley@iee.email> writes:
> On 16/01/2023 13:38, Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Jan 13 2023, Junio C Hamano wrote:
>>
>>> Æ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.
>> Okey, the 6/6 requires ds/omit-trailing-hash-in-index. As both it and
>> the 1-5/6 of this are in "next" now I think it's best that I just submit
>> the 6/6 stand-alone after both of those have graduated.
>>
> micro-nit: The commit message of 5/6 starts "As we'll see in a
> subsequent commit..", which may need a slight tweak if 6/6 becomes 'far
> away' in the commit tree.
Indeed.
I'd use
Hopefully in some not so distant future, we'll get
advantages from always initializing the "repo" member ...
for now.
^ permalink raw reply
* Re: [PATCH] format-patch: unleak "-v <num>"
From: Junio C Hamano @ 2023-01-16 18:35 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <Y8WJnGHs5nM5GwBM@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sun, Jan 15, 2023 at 12:03:39AM -0800, Junio C Hamano wrote:
>
>> The "subject_prefix" member of "struct revision" usually is always
>> set to a borrowed string (either a string literal like "PATCH" that
>> appear in the program text as a hardcoded default, or the value of
>> "format.subjectprefix") and is never freed when the containing
>> revision structure is released. The "-v <num>" codepath however
>> violates this rule and stores a pointer to an allocated string to
>> this member, relinquishing the responsibility to free it when it is
>> done using the revision structure, leading to a small one-time leak.
>>
>> Instead, keep track of the string it allocates to let the revision
>> structure borrow, and clean it up when it is done.
>
> FWIW, this looks obviously correct to me.
>
> The word "unleak" in the subject made me think about UNLEAK(), so this
> is a small tangent. This is exactly the kind of case that I designed
> UNLEAK() for, because the solution really is "while you are assigning to
> X, also keep a copy of the pointer in Y to be freed later".
Yup. I was originally planning to use UNLEAK(), but it felt ugly to
UNLEAK(rev.subject_prefix), as it stores borrowed pointer sometimes
and owned pointer some other times, which is the exact reason why I
started looking for a clean way to plug this leak. So I ended up
with declaring that the member should only store a borrowed pointer.
> And UNLEAK() is just "keep a copy of the pointer in Y to know that we
> _could_ free it later". And of course "do nothing if we are not
> leak-detecting". But since we seem to be moving away from UNLEAK(), and
> since it would not even save any lines here, I'm perfectly happy with
> this solution.
The first sentence needs to be rephrased, as it does not make much
sense to have something usually be X and always be X at the same
time (I'd just remove "always" from there).
Thanks.
^ permalink raw reply
* [PATCH] branch: improve advice when --recurse-submodules fails
From: Philippe Blain via GitGitGadget @ 2023-01-16 17:41 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Philippe Blain, Philippe Blain
From: Philippe Blain <levraiphilippeblain@gmail.com>
'git branch --recurse-submodules start from-here' fails if any submodule
present in 'from-here' is not yet cloned (under
submodule.propagateBranches=true). We then give this advice:
"You may try updating the submodules using 'git checkout from-here && git submodule update --init'"
If 'submodule.recurse' is set, 'git checkout from-here' will also fail since
it will try to recursively checkout the submodules.
Improve the advice by adding '--no-recurse-submodules' to the checkout
command.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
branch: improve advice when --recurse-submodules fails
Hi Glen,
This is a small improvement I thought about when looking at that code
recently.
Cheers,
Philippe.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1464%2Fphil-blain%2Fbranch-recurse-improve-advice-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1464/phil-blain/branch-recurse-improve-advice-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1464
branch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/branch.c b/branch.c
index d182756827f..e5614b53b36 100644
--- a/branch.c
+++ b/branch.c
@@ -756,7 +756,7 @@ void create_branches_recursively(struct repository *r, const char *name,
_("submodule '%s': unable to find submodule"),
submodule_entry_list.entries[i].submodule->name);
if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED))
- advise(_("You may try updating the submodules using 'git checkout %s && git submodule update --init'"),
+ advise(_("You may try updating the submodules using 'git checkout --no-recurse-submodules %s && git submodule update --init'"),
start_commitish);
exit(code);
}
base-commit: 4dbebc36b0893f5094668ddea077d0e235560b16
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 1/1] ref-filter: add new "signature" atom
From: Nsengiyumva Wilberforce @ 2023-01-16 17:38 UTC (permalink / raw)
To: git; +Cc: Nsengiyumva Wilberforce, Hariom Verma, Jaydeep Das,
Christian Couder
In-Reply-To: <20230116173814.11338-1-nsengiyumvawilberforce@gmail.com>
This commit duplicates the code for `signature` atom from pretty.c
to ref-filter.c. This feature will help to get rid of current duplicate
implementation of `signature` atom when unifying implementations by
using ref-filter logic everywhere when ref-filter can do everything
pretty is doing.
Add "signature" atom with `grade`, `signer`, `key`,
`fingerprint`, `primarykeyfingerprint`, `trustlevel` as arguments.
This code and its documentation are inspired by how the %GG, %G?,
%GS, %GK, %GF, %GP, and %GT pretty formats were implemented.
Co-authored-by: Hariom Verma <hariom18599@gmail.com>
Co-authored-by: Jaydeep Das <jaydeepjd.8914@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Nsengiyumva Wilberforce <nsengiyumvawilberforce@gmail.com>
---
Documentation/git-for-each-ref.txt | 27 +++++++
ref-filter.c | 101 +++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 116 +++++++++++++++++++++++++++++
3 files changed, 244 insertions(+)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 6da899c629..9a0be85368 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -212,6 +212,33 @@ symref::
`:lstrip` and `:rstrip` options in the same way as `refname`
above.
+signature::
+ The GPG signature of a commit.
+
+signature:grade::
+ Show "G" for a good (valid) signature, "B" for a bad
+ signature, "U" for a good signature with unknown validity, "X"
+ for a good signature that has expired, "Y" for a good
+ signature made by an expired key, "R" for a good signature
+ made by a revoked key, "E" if the signature cannot be
+ checked (e.g. missing key) and "N" for no signature.
+
+signature:signer::
+ The signer of the GPG signature of a commit.
+
+signature:key::
+ The key of the GPG signature of a commit.
+
+signature:fingerprint::
+ The fingerprint of the GPG signature of a commit.
+
+signature:primarykeyfingerprint::
+ The Primary Key fingerprint of the GPG signature of a commit.
+
+signature:trustlevel::
+ The Trust level of the GPG signature of a commit. Possible
+ outputs are `ultimate`, `fully`, `marginal`, `never` and `undefined`.
+
worktreepath::
The absolute path to the worktree in which the ref is checked
out, if it is checked out in any linked worktree. Empty string
diff --git a/ref-filter.c b/ref-filter.c
index a24324123e..0cba756b18 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -144,6 +144,7 @@ enum atom_type {
ATOM_BODY,
ATOM_TRAILERS,
ATOM_CONTENTS,
+ ATOM_SIGNATURE,
ATOM_RAW,
ATOM_UPSTREAM,
ATOM_PUSH,
@@ -208,6 +209,10 @@ static struct used_atom {
struct email_option {
enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
} email_option;
+ struct {
+ enum { S_BARE, S_GRADE, S_SIGNER, S_KEY,
+ S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL} option;
+ } signature;
struct refname_atom refname;
char *head;
} u;
@@ -394,6 +399,34 @@ static int subject_atom_parser(struct ref_format *format, struct used_atom *atom
return 0;
}
+static int parse_signature_option(const char *arg)
+{
+ if (!arg)
+ return S_BARE;
+ else if (!strcmp(arg, "signer"))
+ return S_SIGNER;
+ else if (!strcmp(arg, "grade"))
+ return S_GRADE;
+ else if (!strcmp(arg, "key"))
+ return S_KEY;
+ else if (!strcmp(arg, "fingerprint"))
+ return S_FINGERPRINT;
+ else if (!strcmp(arg, "primarykeyfingerprint"))
+ return S_PRI_KEY_FP;
+ else if (!strcmp(arg, "trustlevel"))
+ return S_TRUST_LEVEL;
+ return -1;
+}
+
+static int signature_atom_parser(struct ref_format *format UNUSED, struct used_atom *atom,
+ const char *arg, struct strbuf *err){
+ int opt = parse_signature_option(arg);
+ if (opt < 0)
+ return err_bad_arg(err, "signature", arg);
+ atom->u.signature.option = opt;
+ return 0;
+}
+
static int trailers_atom_parser(struct ref_format *format, struct used_atom *atom,
const char *arg, struct strbuf *err)
{
@@ -631,6 +664,7 @@ static struct {
[ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
[ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
[ATOM_CONTENTS] = { "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
+ [ATOM_SIGNATURE] = { "signature", SOURCE_OBJ, FIELD_STR, signature_atom_parser },
[ATOM_RAW] = { "raw", SOURCE_OBJ, FIELD_STR, raw_atom_parser },
[ATOM_UPSTREAM] = { "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
[ATOM_PUSH] = { "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
@@ -1362,6 +1396,72 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void
}
}
+static void grab_signature(struct atom_value *val, int deref, struct object *obj)
+{
+ int i;
+ struct commit *commit = (struct commit *) obj;
+ struct signature_check sigc = { 0 };
+
+ check_commit_signature(commit, &sigc);
+
+ for (i = 0; i < used_atom_cnt; i++) {
+ struct used_atom *atom = &used_atom[i];
+ const char *name = atom->name;
+ struct atom_value *v = &val[i];
+
+ if (!!deref != (*name == '*'))
+ continue;
+ if (deref)
+ name++;
+
+ if (!skip_prefix(name, "signature", &name) || (*name &&
+ *name != ':'))
+ continue;
+ if (!*name)
+ name = NULL;
+ else
+ name++;
+ if (parse_signature_option(name) < 0)
+ continue;
+
+ if (atom->u.signature.option == S_BARE)
+ v->s = xstrdup(sigc.output ? sigc.output: "");
+ else if (atom->u.signature.option == S_SIGNER)
+ v->s = xstrdup(sigc.signer ? sigc.signer : "");
+ else if (atom->u.signature.option == S_GRADE) {
+ switch (sigc.result) {
+ case 'G':
+ switch (sigc.trust_level) {
+ case TRUST_UNDEFINED:
+ case TRUST_NEVER:
+ v->s = xstrfmt("%c", (char)'U');
+ break;
+ default:
+ v->s = xstrfmt("%c", (char)'G');
+ break;
+ }
+ break;
+ case 'B':
+ case 'E':
+ case 'N':
+ case 'X':
+ case 'Y':
+ case 'R':
+ v->s = xstrfmt("%c", (char)sigc.result);
+ }
+ }
+ else if (atom->u.signature.option == S_KEY)
+ v->s = xstrdup(sigc.key ? sigc.key : "");
+ else if (atom->u.signature.option == S_FINGERPRINT)
+ v->s = xstrdup(sigc.fingerprint ? sigc.fingerprint : "");
+ else if (atom->u.signature.option == S_PRI_KEY_FP)
+ v->s = xstrdup(sigc.primary_key_fingerprint ? sigc.primary_key_fingerprint : "");
+ else if (atom->u.signature.option == S_TRUST_LEVEL)
+ v->s = xstrdup(gpg_trust_level_to_str(sigc.trust_level));
+ }
+ signature_check_clear(&sigc);
+}
+
static void find_subpos(const char *buf,
const char **sub, size_t *sublen,
const char **body, size_t *bodylen,
@@ -1555,6 +1655,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s
grab_sub_body_contents(val, deref, data);
grab_person("author", val, deref, buf);
grab_person("committer", val, deref, buf);
+ grab_signature(val, deref, obj);
break;
case OBJ_TREE:
/* grab_tree_values(val, deref, obj, buf, sz); */
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2ae1fc721b..47def9549d 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -6,6 +6,7 @@
test_description='for-each-ref test'
. ./test-lib.sh
+GNUPGHOME_NOT_USED=$GNUPGHOME
. "$TEST_DIRECTORY"/lib-gpg.sh
. "$TEST_DIRECTORY"/lib-terminal.sh
@@ -1464,4 +1465,119 @@ sig_crlf="$(printf "%s" "$sig" | append_cr; echo dummy)"
sig_crlf=${sig_crlf%dummy}
test_atom refs/tags/fake-sig-crlf contents:signature "$sig_crlf"
+GRADE_FORMAT="%(signature:grade)%0a%(signature:key)%0a%(signature:signer)%0a%(signature:fingerprint)%0a%(signature:primarykeyfingerprint)"
+TRUSTLEVEL_FORMAT="%(signature:trustlevel)%0a%(signature:key)%0a%(signature:signer)%0a%(signature:fingerprint)%0a%(signature:primarykeyfingerprint)"
+
+test_expect_success GPG 'show good signature with custom format' '
+ git checkout -b signed &&
+ echo 2 >file && git add file &&
+ test_tick && git commit -S -m initial &&
+ git verify-commit signed 2>out &&
+ cat >expect <<-\EOF &&
+ G
+ 13B6F51ECDDE430D
+ C O Mitter <committer@example.com>
+ 73D758744BE721698EC54E8713B6F51ECDDE430D
+ 73D758744BE721698EC54E8713B6F51ECDDE430D
+ EOF
+ git for-each-ref refs/heads/signed --format="$GRADE_FORMAT" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success GPG 'test signature atom with grade option and bad signature' '
+ git config commit.gpgsign true &&
+ echo 3 >file && test_tick && git commit -a -m "third" --no-gpg-sign &&
+ git tag third-unsigned &&
+
+ test_tick && git rebase -f HEAD^^ && git tag second-signed HEAD^ &&
+ git tag third-signed &&
+
+ git cat-file commit third-signed >raw &&
+ sed -e "s/^third/3rd forged/" raw >forged1 &&
+ FORGED1=$(git hash-object -w -t commit forged1) &&
+ git update-ref refs/tags/third-signed "$FORGED1" &&
+ test_must_fail git verify-commit "$FORGED1" &&
+
+ cat >expect <<-\EOF &&
+ B
+ 13B6F51ECDDE430D
+ C O Mitter <committer@example.com>
+
+
+ EOF
+ git for-each-ref refs/tags/third-signed --format="$GRADE_FORMAT" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success GPG 'show untrusted signature with custom format' '
+ echo 4 >file && test_tick && git commit -a -m fourth -SB7227189 &&
+ git tag signed-fourth &&
+ cat >expect <<-\EOF &&
+ U
+ 65A0EEA02E30CAD7
+ Eris Discordia <discord@example.net>
+ F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
+ D4BE22311AD3131E5EDA29A461092E85B7227189
+ EOF
+ git for-each-ref refs/tags/signed-fourth --format="$GRADE_FORMAT" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success GPG 'show untrusted signature with undefined trust level' '
+ echo 5 >file && test_tick && git commit -a -m fifth -SB7227189 &&
+ git tag fifth-signed &&
+ cat >expect <<-\EOF &&
+ undefined
+ 65A0EEA02E30CAD7
+ Eris Discordia <discord@example.net>
+ F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
+ D4BE22311AD3131E5EDA29A461092E85B7227189
+ EOF
+ git for-each-ref refs/tags/fifth-signed --format="$TRUSTLEVEL_FORMAT" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success GPG 'show untrusted signature with ultimate trust level' '
+ echo 7 >file && test_tick && git commit -a -m "seventh" --no-gpg-sign &&
+ git tag seventh-unsigned &&
+
+ test_tick && git rebase -f HEAD^^ && git tag sixth-signed HEAD^ &&
+ git tag seventh-signed &&
+ cat >expect <<-\EOF &&
+ ultimate
+ 13B6F51ECDDE430D
+ C O Mitter <committer@example.com>
+ 73D758744BE721698EC54E8713B6F51ECDDE430D
+ 73D758744BE721698EC54E8713B6F51ECDDE430D
+ EOF
+ git for-each-ref refs/tags/seventh-signed --format="$TRUSTLEVEL_FORMAT" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success GPG 'show unknown signature with custom format' '
+ cat >expect <<-\EOF &&
+ E
+ 65A0EEA02E30CAD7
+
+
+
+ EOF
+ GNUPGHOME="$GNUPGHOME_NOT_USED" git for-each-ref refs/tags/fifth-signed --format="$GRADE_FORMAT" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success GPG 'show lack of signature with custom format' '
+ echo 8 >file && test_tick && git commit -a -m "eigth unsigned" --no-gpg-sign &&
+ git tag eigth-unsigned &&
+ cat >expect <<-\EOF &&
+ N
+
+
+
+
+ EOF
+ git for-each-ref refs/tags/eigth-unsigned --format="$GRADE_FORMAT" >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.39.0.138.gb334f1a8b9
^ permalink raw reply related
* Re: test_pause giving '__git_ps1: not found' warning
From: Jeff King @ 2023-01-16 17:38 UTC (permalink / raw)
To: Philip Oakley; +Cc: Git List, Philippe Blain, Elijah Newren
In-Reply-To: <8355f48b-dbf9-7cfe-c85c-0d6ca5926c8c@iee.email>
On Sat, Jan 14, 2023 at 05:49:15PM +0000, Philip Oakley wrote:
> On 14/01/2023 14:54, Philip Oakley wrote:
> > I was trying to refine a test_expect_failure test [1] and tried
> > inserting a `test_pause &&` test line [2].
> >
> > I then found, when it paused, I was repeatedly given the warning line
> > /bin/sh: 1: __git_ps1: not found
> > in the terminal until I expected the test shell.
> >
> > my PS1 is working normally in the terminal, but not here. Is this
> > expected, or do I need to set up anything else?
> >
> > Normally I'm on Git for Windows, but this was on my old laptop (Acer
> > 7741 i5 4GB ram..) converted to Ubuntu 20.04, which I use when away.
On Ubuntu, your /bin/sh is likely to be dash. And unless you've set
TEST_SHELL_PATH, that's what test_pause will use.
> My local .bashrc has
>
> . /home/philip/git-completion.bash
> . ~/git-prompt.sh
> export GIT_PS1_SHOWDIRTYSTATE=1
> export PS1='\w$(__git_ps1 " (%s)")\$ '
It's unusual to export PS1. Only the shell needs it. But by exporting
it, the child "dash" will see it, but of course doesn't have the
__git_ps1() function defined. And thus the complaint.
If you stop exporting it, then you'd see dash's regular prompt. Which
fixes the "whoops, __git_ps1() is not defined" problem, but probably is
not quite what you want, if you really wanted a nice prompt.
> Not sure why I have a relative and an absolute path but... , so I'll try
> updating the git-prompt.sh to an absolute path, and if that works, maybe
> think about adding an extra comment to the `test-lib-functions.sh` to note
> the change of HOME and potential '__git_ps1' problem
It looks like test_pause() has various options for retaining shell,
term, home, etc, and includes documentation. I haven't played with them
myself, though. Usually I just use "-i" to stop the test, and then
investigate the result in one of my regular shells (and in the rare case
of "oops, it succeeds when I expect it to fail, I insert "false &&" as
appropriate).
-Peff
^ permalink raw reply
* [PATCH v4 0/1] ref-filter: add new "signature" atom
From: Nsengiyumva Wilberforce @ 2023-01-16 17:38 UTC (permalink / raw)
To: git; +Cc: Nsengiyumva Wilberforce
In-Reply-To: <20230110005251.10539-2-nsengiyumvawilberforce@gmail.com>
This patch is not different from previous version(3), this is because
the previous version(3) could not show the difference from version(2)
of the patch. I will explain every change from the first version
<https://public-inbox.org/git/pull.1452.git.1672102523902.gitgitgadget@gmail.com/>
to version 3
<https://public-inbox.org/git/20230110005251.10539-1-nsengiyumvawilberforce@gmail.com/>
because I first had a big trouble in transitioning from gitgitgadget
to using git send-mail
***THE FOLLOWING ARE THE CHANGES***
a) From the first version to version 2
<https://public-inbox.org/git/pull.1428.git.git.1673254961028.gitgitgadget@gmail.com/>
Version 2 addresses all Junio's comments for version 1, the comments are
here.
<https://public-inbox.org/git/xmqqo7rpvb83.fsf@gitster.g/>
->summary of the changes
i) I changed the commit message to detail more about the feature I am
introducing.
ii) Introduced a new helper function in ref-filter.c called
parse_signature_option() and handled !arg case first.
iii) Used the above helper function to eliminate the repetition that
was in grab_signature() for checking different signature option.
iv) I also moved check_commit_signature(commit, &sigc) out of the
to avoid running GPG twice.still this change is in grab_signature()
in ref-filter.c.
v) add a new test in t6300 to test bare signature atom(%(signature))
since I had missed it
NB: I did not change the parser function name as he suggested, I
think my commit message was misleading.
b) from version 2 to version 3
<https://public-inbox.org/git/20230110005251.10539-2-nsengiyumvawilberforce@gmail.com/>
->summary of changes
i) Got rid of test for bare signature atom. This is because the test was
passing for some CI tests(different machines) and some others were
failing.
Best Regards,
Wilberforce
Nsengiyumva Wilberforce (1):
ref-filter: add new "signature" atom
Documentation/git-for-each-ref.txt | 27 +++++++
ref-filter.c | 101 +++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 116 +++++++++++++++++++++++++++++
3 files changed, 244 insertions(+)
Range-diff against v3:
1: ce51d8e79e = 1: ce51d8e79e ref-filter: add new "signature" atom
--
2.39.0.138.gb334f1a8b9
^ permalink raw reply
* Re: [PATCH] format-patch: unleak "-v <num>"
From: Jeff King @ 2023-01-16 17:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqv8l8gr6s.fsf@gitster.g>
On Sun, Jan 15, 2023 at 12:03:39AM -0800, Junio C Hamano wrote:
> The "subject_prefix" member of "struct revision" usually is always
> set to a borrowed string (either a string literal like "PATCH" that
> appear in the program text as a hardcoded default, or the value of
> "format.subjectprefix") and is never freed when the containing
> revision structure is released. The "-v <num>" codepath however
> violates this rule and stores a pointer to an allocated string to
> this member, relinquishing the responsibility to free it when it is
> done using the revision structure, leading to a small one-time leak.
>
> Instead, keep track of the string it allocates to let the revision
> structure borrow, and clean it up when it is done.
FWIW, this looks obviously correct to me.
The word "unleak" in the subject made me think about UNLEAK(), so this
is a small tangent. This is exactly the kind of case that I designed
UNLEAK() for, because the solution really is "while you are assigning to
X, also keep a copy of the pointer in Y to be freed later".
And UNLEAK() is just "keep a copy of the pointer in Y to know that we
_could_ free it later". And of course "do nothing if we are not
leak-detecting". But since we seem to be moving away from UNLEAK(), and
since it would not even save any lines here, I'm perfectly happy with
this solution.
-Peff
^ permalink raw reply
* [PATCH] builtin/checkout: check the branch used in -B with worktrees
From: Carlo Marcelo Arenas Belón @ 2023-01-16 17:28 UTC (permalink / raw)
To: git; +Cc: vustthat, sunshine, pclouds, Carlo Marcelo Arenas Belón
When multiple worktrees are being used, checkout/switch check
that the target branch is not already checked out with code that
evolved from 8d9fdd7087 (worktree.c: check whether branch is rebased
in another worktree, 2016-04-22), but that logic wasn't valid for
-B/-C
Avoid reusing the same `branch_info` structure for the checks and
assumming that it will be rejected later if is a new branch that
already exists as that doesn't apply to -B/-C.
Reported-by: Jinwook Jeong <vustthat@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
builtin/checkout.c | 22 ++++++++++++++++------
t/t2400-worktree-add.sh | 14 ++++++++++++++
2 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3fa29a08ee..94dcd617ef 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)
}
static int checkout_branch(struct checkout_opts *opts,
- struct branch_info *new_branch_info)
+ struct branch_info *new_branch_info,
+ struct branch_info *check_branch_info)
{
if (opts->pathspec.nr)
die(_("paths cannot be used with switching branches"));
@@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts,
if (!opts->can_switch_when_in_progress)
die_if_some_operation_in_progress();
- if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
- !opts->ignore_other_worktrees) {
+ if (check_branch_info->path && !opts->force_detach && !opts->ignore_other_worktrees) {
int flag;
char *head_ref = resolve_refdup("HEAD", 0, NULL, &flag);
if (head_ref &&
- (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))
- die_if_checked_out(new_branch_info->path, 1);
+ (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path)))
+ die_if_checked_out(check_branch_info->path, 1);
free(head_ref);
}
@@ -1628,6 +1628,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
struct branch_info *new_branch_info)
{
int parseopt_flags = 0;
+ struct branch_info check_branch_info = { 0 };
opts->overwrite_ignore = 1;
opts->prefix = prefix;
@@ -1739,6 +1740,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
!opts->new_branch;
int n = parse_branchname_arg(argc, argv, dwim_ok,
new_branch_info, opts, &rev);
+ check_branch_info = *new_branch_info;
argv += n;
argc -= n;
} else if (!opts->accept_ref && opts->from_treeish) {
@@ -1751,8 +1753,16 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
opts, &rev,
opts->from_treeish);
+ check_branch_info = *new_branch_info;
if (!opts->source_tree)
die(_("reference is not a tree: %s"), opts->from_treeish);
+ } else if (opts->new_branch) {
+ struct object_id rev;
+
+ if (!get_oid_mb(opts->new_branch, &rev))
+ setup_new_branch_info_and_source_tree(&check_branch_info,
+ opts, &rev,
+ opts->new_branch);
}
if (argc) {
@@ -1819,7 +1829,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
if (opts->patch_mode || opts->pathspec.nr)
return checkout_paths(opts, new_branch_info);
else
- return checkout_branch(opts, new_branch_info);
+ return checkout_branch(opts, new_branch_info, &check_branch_info);
}
int cmd_checkout(int argc, const char **argv, const char *prefix)
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index d587e0b20d..283ba7607e 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -125,6 +125,13 @@ test_expect_success 'die the same branch is already checked out' '
)
'
+test_expect_success 'die the same branch is already checked out (checkout -B)' '
+ (
+ cd here &&
+ test_must_fail git checkout -B newmain
+ )
+'
+
test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '
head=$(git -C there rev-parse --git-path HEAD) &&
ref=$(git -C there symbolic-ref HEAD) &&
@@ -147,6 +154,13 @@ test_expect_success 'not die on re-checking out current branch' '
)
'
+test_expect_success 'not die on re-checking out current branch (checkout -B)' '
+ (
+ cd there &&
+ git checkout -B newmain
+ )
+'
+
test_expect_success '"add" from a bare repo' '
(
git clone --bare . bare &&
--
2.39.0.1.g119e9c6876.dirty
^ permalink raw reply related
* Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
From: Jeff King @ 2023-01-16 17:27 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Junio C Hamano, git, Ramsay Jones
In-Reply-To: <230116.86edruzk5m.gmgdl@evledraar.gmail.com>
On Mon, Jan 16, 2023 at 02:06:50PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > -static long get_curl_allowed_protocols(int from_user)
> > +static void proto_list_append(struct strbuf *list_str, const char *proto_str,
> > + long *list_bits, long proto_bits)
> > +{
> > + *list_bits |= proto_bits;
> > + if (list_str) {
> > + if (list_str->len)
> > + strbuf_addch(list_str, ',');
> > + strbuf_addstr(list_str, proto_str);
> > + }
> > +}
>
> Nit: It would be nice (especially in this even smaller function) to
> carry forward the name the parent get_curl_allowed_protocols() uses,
> i.e. just "list", not "list_str", ditto "proto" rather than "proto_str".
I think it gets confusing in this function, then, because you have both
types. If anything, the sin is in the caller which uses "list" and
"allowed_protocols". I had originally written that as "list" and "bits",
but I left "bits" as "allowed_protocols" to reduce the size of the diff.
Maybe that was a bad choice.
Likewise, the caller could just do the bitwise-OR inline, like:
if (is_transported_allowed("http", from_user)) {
bits |= CURLPROTO_HTTP;
proto_list_append(list, "http");
}
but that makes the diff bigger (the whole function body is replaced,
because the "if" lines, which now have a "{", are no longer unchanged
context). But again, maybe optimizing for a small diff is a bad idea if
the resulting code is harder to follow (I didn't think it was, but then
I also wrote it).
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
From: Jeff King @ 2023-01-16 17:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git, Ramsay Jones
In-Reply-To: <xmqqzgaicvmj.fsf@gitster.g>
On Mon, Jan 16, 2023 at 08:05:56AM -0800, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > +#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR (LIBCURL_VERSION_NUM >= 0x075500)
> > +#if GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
> > +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR CURLOPT_REDIR_PROTOCOLS_STR
> > +#define GIT_CURLOPT_PROTOCOLS_STR CURLOPT_PROTOCOLS_STR
> > +#else
> > +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR 0
> > +#define GIT_CURLOPT_PROTOCOLS_STR 0
> > #endif
>
> I find it a bit ugly that CURLOPT_* being used are all non-zero, but
> it may be true in practice.
I don't think it matters what the dummy values are, at least from a
run-time perspective. We'd only feed them to curl inside the "if (0)"
block. They just need to be non-empty so the compiler is happy.
But I do think there's the possibility of compile-time confusion. Curl
is doing macro magic for deprecation and type-checking here, and it's
not clear how it will handle the magic "0" values, even if we know
the code will never actually run.
> I somehow find the above over-engineered solution looking for a
> problem. Conditional compilation might be ugly, but what is uglier
> is a conditional compilation hidden behind what pretends to be a
> runtime conditional but gets optimized away at compile time. Also,
> when the non _STR variant changes status from deprecated to removed,
> the code will cease to build, so I am not sure if the above is the
> whole story. You'd also need dummy definitions for them when the
> version of cURL is advanced enough.
>
> It is true that with the above we will always pass both sides to the
> compiler, which may be an upside, but at the same time doesn't it
> make it harder to notice and remove the support of the older side
> once the time comes?
I likewise found that solution in an uncanny valley of over-engineering
for not much gain.
If you really want to over-engineer, then something like the patch below
at least makes the conditional part simpler, because it shares the
curl_easy_setopt() calls. You can actually take it even further and
abstract the declaration of the strbuf completely (and thus omit it when
it is not going to be used), but it makes the code even more obscure.
I dunno. My original patch tried to err on the side of
simple-and-stupid. If we don't mind repeating the list of
http/https/ftp/ftps, then it can be even simpler (and stupider). But the
eventual cleanup becomes really easy, then: just delete the #else half
of each #ifdef.
diff --git a/git-curl-compat.h b/git-curl-compat.h
index 56a83b6bbd..e545d26dfa 100644
--- a/git-curl-compat.h
+++ b/git-curl-compat.h
@@ -126,4 +126,20 @@
#define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS
#endif
+/**
+ * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,
+ * released in August 2022.
+ */
+#if LIBCURL_VERSION_NUM >= 0x075500
+#define GIT_CURL_PROTOCOL_TYPE const char *
+#define GIT_CURL_PROTOCOL_RETURN(list, bits) list->buf
+#define GIT_CURL_OPT_PROTOCOLS CURLOPT_PROTOCOLS_STR
+#define GIT_CURL_OPT_REDIR_PROTOCOLS CURLOPT_REDIR_PROTOCOLS_STR
+#else
+#define GIT_CURL_PROTOCOL_TYPE long
+#define GIT_CURL_PROTOCOL_RETURN(list, bits) bits
+#define GIT_CURL_OPT_PROTOCOLS CURLOPT_PROTOCOLS
+#define GIT_CURL_OPT_REDIR_PROTOCOLS CURLOPT_REDIR_PROTOCOLS
+#endif
+
#endif
diff --git a/http.c b/http.c
index ca0fe80ddb..e5ed3521db 100644
--- a/http.c
+++ b/http.c
@@ -764,20 +764,32 @@ void setup_curl_trace(CURL *handle)
curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
}
-static long get_curl_allowed_protocols(int from_user)
+static void proto_list_append(struct strbuf *list_str, const char *proto_str,
+ long *list_bits, long proto_bits)
{
- long allowed_protocols = 0;
+ *list_bits |= proto_bits;
+ if (list_str) {
+ if (list_str->len)
+ strbuf_addch(list_str, ',');
+ strbuf_addstr(list_str, proto_str);
+ }
+}
+
+static GIT_CURL_PROTOCOL_TYPE get_curl_allowed_protocols(struct strbuf *list,
+ int from_user)
+{
+ long bits = 0;
if (is_transport_allowed("http", from_user))
- allowed_protocols |= CURLPROTO_HTTP;
+ proto_list_append(list, "http", &bits, CURLPROTO_HTTP);
if (is_transport_allowed("https", from_user))
- allowed_protocols |= CURLPROTO_HTTPS;
+ proto_list_append(list, "https", &bits, CURLPROTO_HTTPS);
if (is_transport_allowed("ftp", from_user))
- allowed_protocols |= CURLPROTO_FTP;
+ proto_list_append(list, "ftp", &bits, CURLPROTO_FTP);
if (is_transport_allowed("ftps", from_user))
- allowed_protocols |= CURLPROTO_FTPS;
+ proto_list_append(list, "ftps", &bits, CURLPROTO_FTPS);
- return allowed_protocols;
+ return GIT_CURL_PROTOCOL_RETURN(list, bits);
}
#ifdef GIT_CURL_HAVE_CURL_HTTP_VERSION_2
@@ -921,10 +933,17 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
- curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
- get_curl_allowed_protocols(0));
- curl_easy_setopt(result, CURLOPT_PROTOCOLS,
- get_curl_allowed_protocols(-1));
+
+ {
+ struct strbuf buf = STRBUF_INIT;
+
+ curl_easy_setopt(result, GIT_CURL_OPT_REDIR_PROTOCOLS,
+ get_curl_allowed_protocols(&buf, 0));
+ curl_easy_setopt(result, GIT_CURL_OPT_PROTOCOLS,
+ get_curl_allowed_protocols(&buf, -1));
+ strbuf_release(&buf);
+ }
+
if (getenv("GIT_CURL_VERBOSE"))
http_trace_curl_no_data();
setup_curl_trace(result);
^ permalink raw reply related
* Re: [PATCH 3/4] ls-files: clarify descriptions of status tags for -t
From: ZheNing Hu @ 2023-01-16 17:21 UTC (permalink / raw)
To: Elijah Newren; +Cc: Elijah Newren via GitGitGadget, git
In-Reply-To: <CABPp-BFFfDPT1x9E4bucuQnyVrWacego2agzyqjT7h+wQ=xpUg@mail.gmail.com>
Elijah Newren <newren@gmail.com> 于2023年1月15日周日 04:27写道：
>
> On Sat, Jan 14, 2023 at 12:27 AM ZheNing Hu <adlternative@gmail.com> wrote:
> >
> > Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> 于2023年1月13日周五 12:41写道：
> > >
> > > From: Elijah Newren <newren@gmail.com>
> > >
> > > Much like the file selection options we tweaked in the last commit, the
> > > status tags printed with -t had descriptions that were easy to
> > > misunderstand, and for many of the same reasons. Clarify them.
> > >
> > > Also, while at it, remove the "semi-deprecated" comment for "git
> > > ls-files -t". The -t option was marked as semi-deprecated in 5bc0e247c4
> > > ("Document ls-files -t as semi-obsolete.", 2010-07-28) because:
> > >
> > > "git ls-files -t" is [...] badly documented, hence we point the
> > > users to superior alternatives.
> > > The feature is marked as "semi-obsolete" but not "scheduled for removal"
> > > since it's a plumbing command, scripts might use it, and Git testsuite
> > > already uses it to test the state of the index.
> > >
> > > Marking it as obsolete because it was easily misunderstood, which I
> > > think was primarily due to documentation problems, is one strategy, but
> > > I think fixing the documentation is a better option. Especially since
> > > in the intervening time, "git ls-files -t" has become heavily used by
> > > sparse-checkout users where the same confusion just doesn't apply.
> > >
> > > Signed-off-by: Elijah Newren <newren@gmail.com>
> > > ---
> > > Documentation/git-ls-files.txt | 28 +++++++++++++++-------------
> > > 1 file changed, 15 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> > > index f89ab1bfc98..3886d58d178 100644
> > > --- a/Documentation/git-ls-files.txt
> > > +++ b/Documentation/git-ls-files.txt
> > > @@ -137,25 +137,27 @@ OPTIONS
> > > with `-s` or `-u` options does not make any sense.
> > >
> > > -t::
> > > - This feature is semi-deprecated. For scripting purpose,
> > > - linkgit:git-status[1] `--porcelain` and
> > > + Show status tags together with filenames. Note that for
> > > + scripting purposes, linkgit:git-status[1] `--porcelain` and
> > > linkgit:git-diff-files[1] `--name-status` are almost always
> > > superior alternatives, and users should look at
> > > linkgit:git-status[1] `--short` or linkgit:git-diff[1]
> > > `--name-status` for more user-friendly alternatives.
> > > +
> > > --
> > > -This option identifies the file status with the following tags (followed by
> > > -a space) at the start of each line:
> > > -
> > > - H:: cached
> > > - S:: skip-worktree
> > > - M:: unmerged
> > > - R:: removed/deleted
> > > - C:: modified/changed
> > > - K:: to be killed
> > > - ?:: other
> > > - U:: resolve-undo
> > > +This option provides a reason for showing each filename, in the form
> > > +of a status tag (which is followed by a space and then the filename).
> > > +The status tags are all single characters from the following list:
> > > +
> > > + H:: tracked file that is not either unmerged or skip-worktree
> > > + S:: tracked file that is skip-worktree
> > > + M:: tracked file that is unmerged
> > > + R:: tracked file with unstaged removal/deletion
> > > + C:: tracked file with unstaged modification/change
> > > + K:: untracked paths which are part of file/directory conflicts
> > > + which prevent checking out tracked files
> > > + ?:: untracked file
> > > + U:: file with resolve-undo information
> > > --
> > >
> >
> > Good to see these tags describe are changed, especially "K" (reader
> > don't know what is "to be killed")
> >
> > Maybe we should mention which option will output these tags?
> > e.g. default -> "H"/"S" ,`--other` -> "?", `--modified` -> "C",
> > `--killed` -> "K"...
>
> We could, but...
>
> * It's H/S/M, not just H/S that is shown by default.
> * It gets weird because other options aren't added to the default,
> so if someone specifies "-m" then suddenly H/S/M go away...unless they
> also specify "-c".
>
> Trying to explain all that feels like we're getting close to repeating
> the documentation of the individual options. But maybe we could just
> ignore everything around default behavior and find a way to be brief
> such as with:
>
> Note that H, S, and M entries are shown with --cached; R entries
> are shown with --deleted, C entries are shown with --modified, K
> entries are shown with --killed, ? entries are shown with
> --others, and U entries are shown with --resolve-undo.
>
What you mean is that each tag will appear in which commands, rather than
each command will have which tags. I think this segment is pretty good.
> I'm not sure if I like the documentation better with or without this
> added paragraph. What do others think?
^ permalink raw reply
* Re: [PATCH] ci: do not die on deprecated-declarations warning
From: Jeff King @ 2023-01-16 17:13 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Daniel Stenberg, Patrick Monnerat, git
In-Reply-To: <6d826cd9-e447-8df9-83b9-8deb32d6063d@ramsayjones.plus.com>
On Mon, Jan 16, 2023 at 12:39:39AM +0000, Ramsay Jones wrote:
> > By the way, it seemed odd to me that this failed in just the linux-musl
> > job, and not elsewhere (nor on my development machine, which has curl
> > 7.87.0). It looks like there's a bad interaction within curl between the
> > typecheck and deprecation macros. Here's a minimal reproduction:
> >
> > -- >8 --
> > cat >foo.c <<-\EOF
> > #include <curl/curl.h>
> > void foo(CURL *c)
> > {
> > curl_easy_setopt(c, CURLOPT_PROTOCOLS, 0);
> > }
> > EOF
> >
> > # this will complain about deprecated CURLOPT_PROTOCOLS
> > gcc -DCURL_DISABLE_TYPECHECK -Wdeprecated-declarations -c foo.c
> >
> > # this will not
> > gcc -Wdeprecated-declarations -c foo.c
> > -- 8< --
>
> FYI, I just tried this on cygwin and both gcc invocations above
> complain about deprecated CURLOPT_PROTOCOLS. (On Linux I have
> curl 7.81.0, so I can't test there).
It did work on Linux for me, of course, using 7.87.0. But curiously this
morning it behaved differently!
In the meantime, Debian's libcurl packaging picked up this upstream
patch (and I upgraded):
https://github.com/curl/curl/commit/e2aed004302e51cfa5b6ce8c8ab65ef92aa83196
and now the deprecation warning happens consistently. So I think on the
curl side there is nothing left to do.
-Peff
^ permalink raw reply
* Re: [PATCH 2/4] ls-files: clarify descriptions of file selection options
From: ZheNing Hu @ 2023-01-16 17:12 UTC (permalink / raw)
To: Elijah Newren; +Cc: Elijah Newren via GitGitGadget, git
In-Reply-To: <CABPp-BFuZK2yap_VXWPo8qHvJ+E9V7v2UAZ9tAaQcX8zc-daDA@mail.gmail.com>
Elijah Newren <newren@gmail.com> 于2023年1月15日周日 03:42写道：
>
> On Sat, Jan 14, 2023 at 12:21 AM ZheNing Hu <adlternative@gmail.com> wrote:
> >
> > Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> 于2023年1月13日周五 12:41写道：
> [...]
> > > diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> > > index cb071583f8b..f89ab1bfc98 100644
> > > --- a/Documentation/git-ls-files.txt
> > > +++ b/Documentation/git-ls-files.txt
> > > @@ -29,21 +29,26 @@ This merges the file listing in the index with the actual working
> > > directory list, and shows different combinations of the two.
> > >
> > > One or more of the options below may be used to determine the files
> > > -shown:
> > > +shown, and each file may be printed multiple times if there are
> > > +multiple entries in the index or multiple statuses are applicable for
> > > +the relevant file selection options.
> > >
> >
> > `--deduplicate` option can be used to remove deduped output.
>
> Yes, I'm aware.
>
> If you're suggesting adding this text at this point in the document,
> it occurred to me already, but I chose not to put it here. The reason
> is that this is a brief synopsis. The "relevant file selection
> options" of this brief synopsis could also be expanded to mention what
> they are or what the default selection is or whatever. But folks can
> read on to learn that `deduplicate` can be used to remove duplicate
> options. Likewise, anyone who reads the text about "relevant file
> selections" and wants to learn more is inclined to read on to the
> other options to find out.
>
> In contrast, no one will be motivated to read on to find out that
> files can be printed multiple times if we don't mention it right here.
> And they are likely to get confused when it happens, thinking it is a
> bug (in fact, I can point out emails from the archives where that has
> happened). Without mentioning the possibility of multiple files at
> this point, we have a discoverability problem.
>
> There is no similar discoverability and negative-surprise problem I
> can think of by omitting other details, so there is no need to expand
> this brief synopsis any further.
>
Well, you are right. It may be better to be concise here, telling users too
much will make it difficult to read.
> The one place we could potentially change thing that might help, is
> moving the text about -c being the default from under the -c option
> and putting it here. That's a toss-up to me, but for now I elected to
> keep it where it is.
>
I think it's fine to do this or not.
> > > OPTIONS
> > > -------
> > > -c::
> > > --cached::
> > > - Show cached files in the output (default)
> > > + Show all files cached in Git's index, i.e. all tracked files.
> > > + (This is the default if no -c/-s/-d/-o/-u/-k/-m/--resolve-undo
> > > + options are specified.)
> > >
> > > -d::
> > > --deleted::
> > > - Show deleted files in the output
> > > + Show files with an unstaged deletion
> > >
> >
> > This is a nice fix: make it clear to the user that only files in the
> > working tree are deleted, not in the index.
> >
> > > -m::
> > > --modified::
> > > - Show modified files in the output
> > > + Show files with an unstaged modification (note that an unstaged
> > > + deletion also counts as an unstaged modification)
> > >
> >
> > Good to mention that deleted files are also modified, otherwise no one
> > looking at the documentation would know that.
> >
> > > -o::
> > > --others::
> [...]
>
> Thanks for taking a look!
^ permalink raw reply
* Re: [PATCH v2 0/6] cache API: always have a "istate->repo"
From: Philip Oakley @ 2023-01-16 16:53 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, Junio C Hamano
Cc: git, Derrick Stolee, Victoria Dye, Jeff Hostetler
In-Reply-To: <230116.86a62izjg5.gmgdl@evledraar.gmail.com>
On 16/01/2023 13:38, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Jan 13 2023, Junio C Hamano wrote:
>
>> Æ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.
> Okey, the 6/6 requires ds/omit-trailing-hash-in-index. As both it and
> the 1-5/6 of this are in "next" now I think it's best that I just submit
> the 6/6 stand-alone after both of those have graduated.
>
micro-nit: The commit message of 5/6 starts "As we'll see in a
subsequent commit..", which may need a slight tweak if 6/6 becomes 'far
away' in the commit tree.
--
Philip
^ permalink raw reply
* Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
From: Junio C Hamano @ 2023-01-16 16:26 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Jeff King, git, Ramsay Jones
In-Reply-To: <xmqqzgaicvmj.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> +#if GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
>> +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR CURLOPT_REDIR_PROTOCOLS_STR
>> +#define GIT_CURLOPT_PROTOCOLS_STR CURLOPT_PROTOCOLS_STR
>> +#else
>> +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR 0
>> +#define GIT_CURLOPT_PROTOCOLS_STR 0
>> #endif
>
> I find it a bit ugly that CURLOPT_* being used are all non-zero, but
> it may be true in practice.
Sorry for making the first half of the sentence unintelligible. I
meant to say that it is ugly that the correctness of the solution
depends on the real value for these macros being non-zero.
^ 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