* Re: [PATCH] t3200: clean side effect of git checkout --orphan
From: Eric Sunshine @ 2020-08-30 5:35 UTC (permalink / raw)
To: Aaron Lipman; +Cc: Git List
In-Reply-To: <20200829225648.11971-1-alipman88@gmail.com>
On Sat, Aug 29, 2020 at 6:57 PM Aaron Lipman <alipman88@gmail.com> wrote:
> The "refuse --edit-description on unborn branch for now" test in t3200
> switches to an orphan branch, causing subsequent git commands
> referencing HEAD to fail. Avoid this side-effect by switching back to
> master after that test finishes.
>
> This has gone undetected, as the next effected test expects failure -
> but it currently fails for the wrong reason.
s/effected/affected
In fact, the three tests following the orphan test all expect failure
(though I didn't check if they also fail for the wrong reason), and
the following test which doesn't expect failure has an explicit "git
checkout master" early on, which explains why it was not impacted by
this problem.
> Verbose output of the next test referencing HEAD,
> "--merged is incompatible with --no-merged":
>
> fatal: malformed object name HEAD
>
> Which this commit corrects to:
>
> error: option `no-merged' is incompatible with --merged
>
> Signed-off-by: Aaron Lipman <alipman88@gmail.com>
Description and actual fix make perfect sense.
^ permalink raw reply
* Re: [PATCH] Avoid infinite loop in malformed packfiles
From: ori @ 2020-08-30 3:33 UTC (permalink / raw)
To: gitster, peff; +Cc: l.s.r, ori, git
In-Reply-To: <xmqqft8b3fj4.fsf@gitster.c.googlers.com>
> Jeff King <peff@peff.net> writes:
>
>> It may be hard to test, as I suspect modern versions of Git are not
>> happy to create such a deep chain. We could test with a lowered value of
>> the config option, though.
>
> Yes, that was what I meant. Start from a 1KB text, create 50
> revisions of the file by adding a single line at its end at a time,
> pack with depth limit of 100, and then see "git log -p" die when the
> allowed max lowered to 10, or something like that.
Sorry about the delay -- most of my time to poke at this is over the weekend.
Will that work? I'd expect that modern pack files end up being
offset deltas, rather than reference deltas.
^ permalink raw reply
* [PATCH] t3200: clean side effect of git checkout --orphan
From: Aaron Lipman @ 2020-08-29 22:56 UTC (permalink / raw)
To: git; +Cc: Aaron Lipman
The "refuse --edit-description on unborn branch for now" test in t3200
switches to an orphan branch, causing subsequent git commands
referencing HEAD to fail. Avoid this side-effect by switching back to
master after that test finishes.
This has gone undetected, as the next effected test expects failure -
but it currently fails for the wrong reason.
Verbose output of the next test referencing HEAD,
"--merged is incompatible with --no-merged":
fatal: malformed object name HEAD
Which this commit corrects to:
error: option `no-merged' is incompatible with --merged
Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
We might considered updating "--merged is incompatible with --no-merged"
tests to not only test for failure, but check the contents of the error
message to ensure they fail for the correct reason, e.g.:
test_expect_success '--merged is incompatible with --no-merged' '
test_must_fail git branch --merged HEAD --no-merged HEAD 2>error &&
test_i18ngrep "is incompatible with --merged" error
'
However, I intend to submit a patch enabling ref-filter to accept
multiple merged/no-merged filters, which would make said updates
irrelevant.
t/t3200-branch.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 4c0734157b..028c88d1b2 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1287,6 +1287,7 @@ test_expect_success 'detect typo in branch name when using --edit-description' '
'
test_expect_success 'refuse --edit-description on unborn branch for now' '
+ test_when_finished "git checkout master" &&
write_script editor <<-\EOF &&
echo "New contents" >"$1"
EOF
--
2.24.3 (Apple Git-128)
^ permalink raw reply related
* [PATCH v2] revision: add separate field for "-m" of "diff-index -m"
From: Sergey Organov @ 2020-08-29 20:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Sergey Organov
In-Reply-To: <20200829194634.23306-1-sorganov@gmail.com>
Historically, in "diff-index -m", "-m" does not mean "do not ignore merges", but
"match missing". Despite this, diff-index abuses 'ignore_merges' field being set
by "-m", that in turn causes more troubles.
Add separate 'diff_index_match_missing' field for diff-index to use and set it
when we encounter "-m" option. This field won't then be cleared when primary
meaning of "-m" is reverted (e.g., by "--no-diff-merges"), nor it will be
affected by future option(s) that might drive 'ignore_merges' field.
Use this new field from diff-lib:do_oneway_diff() instead of abusing
'ignore_merges' field.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
v2: rebased from 'maint' onto 'master'
diff-lib.c | 10 ++--------
revision.c | 6 ++++++
revision.h | 1 +
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/diff-lib.c b/diff-lib.c
index 50521e2093fc..f2aee78e7aa2 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -405,14 +405,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
/* if the entry is not checked out, don't examine work tree */
cached = o->index_only ||
(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));
- /*
- * Backward compatibility wart - "diff-index -m" does
- * not mean "do not ignore merges", but "match_missing".
- *
- * But with the revision flag parsing, that's found in
- * "!revs->ignore_merges".
- */
- match_missing = !revs->ignore_merges;
+
+ match_missing = revs->diff_index_match_missing;
if (cached && idx && ce_stage(idx)) {
struct diff_filepair *pair;
diff --git a/revision.c b/revision.c
index 96630e31867d..64b16f7d1033 100644
--- a/revision.c
+++ b/revision.c
@@ -2345,6 +2345,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->diffopt.flags.tree_in_recursive = 1;
} else if (!strcmp(arg, "-m")) {
revs->ignore_merges = 0;
+ /*
+ * Backward compatibility wart - "diff-index -m" does
+ * not mean "do not ignore merges", but "match_missing",
+ * so set separate flag for it.
+ */
+ revs->diff_index_match_missing = 1;
} else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
if (!strcmp(optarg, "off")) {
revs->ignore_merges = 1;
diff --git a/revision.h b/revision.h
index c1e5bcf139d7..5ae8254ffaed 100644
--- a/revision.h
+++ b/revision.h
@@ -188,6 +188,7 @@ struct rev_info {
unsigned int diff:1,
full_diff:1,
show_root_diff:1,
+ diff_index_match_missing:1,
no_commit_id:1,
verbose_header:1,
combine_merges:1,
--
2.25.1
^ permalink raw reply related
* [PATCH] revision: add separate field for "-m" of "diff-index -m"
From: Sergey Organov @ 2020-08-29 19:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Sergey Organov
Historically, in "diff-index -m", "-m" does not mean "do not ignore merges", but
"match missing". Despite this, diff-index abuses 'ignore_merges' field being set
by "-m", that in turn causes more troubles.
Add separate 'diff_index_match_missing' field for diff-index to use and set it
when we encounter "-m" option. This field won't then be cleared when primary
meaning of "-m" is reverted (e.g., by "--no-diff-merges"), nor it will be
affected by future option(s) that might drive 'ignore_merges' field.
Use this new field from diff-lib:do_oneway_diff() instead of abusing
'ignore_merges' field.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
diff-lib.c | 10 ++--------
revision.c | 6 ++++++
revision.h | 1 +
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/diff-lib.c b/diff-lib.c
index 25fd2dee19c4..8c40111b5f35 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -404,14 +404,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
/* if the entry is not checked out, don't examine work tree */
cached = o->index_only ||
(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));
- /*
- * Backward compatibility wart - "diff-index -m" does
- * not mean "do not ignore merges", but "match_missing".
- *
- * But with the revision flag parsing, that's found in
- * "!revs->ignore_merges".
- */
- match_missing = !revs->ignore_merges;
+
+ match_missing = revs->diff_index_match_missing;
if (cached && idx && ce_stage(idx)) {
struct diff_filepair *pair;
diff --git a/revision.c b/revision.c
index 6aa7f4f56755..95f9cfddb02c 100644
--- a/revision.c
+++ b/revision.c
@@ -2325,6 +2325,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->diffopt.flags.tree_in_recursive = 1;
} else if (!strcmp(arg, "-m")) {
revs->ignore_merges = 0;
+ /*
+ * Backward compatibility wart - "diff-index -m" does
+ * not mean "do not ignore merges", but "match_missing",
+ * so set separate flag for it.
+ */
+ revs->diff_index_match_missing = 1;
} else if (!strcmp(arg, "-c")) {
revs->diff = 1;
revs->dense_combined_merges = 0;
diff --git a/revision.h b/revision.h
index f412ae85ebaf..979dddbdaf7c 100644
--- a/revision.h
+++ b/revision.h
@@ -188,6 +188,7 @@ struct rev_info {
unsigned int diff:1,
full_diff:1,
show_root_diff:1,
+ diff_index_match_missing:1,
no_commit_id:1,
verbose_header:1,
ignore_merges:1,
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v6 00/13] Finish converting git bisect to C part 2
From: Junio C Hamano @ 2020-08-29 19:31 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git
In-Reply-To: <20200828124617.60618-1-mirucam@gmail.com>
Miriam Rubio <mirucam@gmail.com> writes:
> These patches correspond to a second part of patch series
> of Outreachy project "Finish converting `git bisect` from shell to C"
> started by Pranit Bauva and Tanushree Tumane
> (https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and
> continued by me.
>
> These patch series emails were generated from:
> https://gitlab.com/mirucam/git/commits/git-bisect-work-part2-v6ps.
>
> I would like to thank the reviewers for their help.
>
> General changes
> ---------------
>
> * Rebase on master branch: e9b77c84a0 (Tenth batch, 2020-08-24)
Was this done because the updated series newly depends on something
that was not available in f402ea68 (The fifth batch, 2020-06-25)[*]
but is available in e9b77c84a0 (Tenth batch, 2020-08-24), and/or is
there a change between the fifth and tenth batch that conflicts with
the old iteration?
Side note: the previous iteration, which is queued in
'seen', was applied on top of f402ea68.
Updating to a new base is not by itself wrong, but please also
describe the reason why the topic was rebased, perhaps like
* Rebased on e9b77c84a0 (Tenth batch, 2020-08-24), to build on the
strvec API (instead of argv_array) and adjust to the codebase
after the "--first-parent" feature was added.
or something like that.
> [6/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
> functions in C
>
> * Change `file_exists(git_path_bisect_head())` to a refs API call.
Very much appreciated.
Thanks.
^ permalink raw reply
* Re: [PATCH v6 06/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
From: Junio C Hamano @ 2020-08-29 19:31 UTC (permalink / raw)
To: Miriam Rubio
Cc: git, Pranit Bauva, Lars Schneider, Christian Couder,
Johannes Schindelin, Tanushree Tumane
In-Reply-To: <20200828124617.60618-7-mirucam@gmail.com>
Miriam Rubio <mirucam@gmail.com> writes:
> diff --git a/bisect.c b/bisect.c
> index c6aba2b9f2..f0fca5c6f3 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -988,6 +988,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
> * the bisection process finished successfully.
> * In this case the calling function or command should not turn a
> * BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND return code into an error or a non zero exit code.
> + *
> + * Checking BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND
> + * in bisect_helper::bisect_next() and only transforming it to 0 at
> + * the end of bisect_helper::cmd_bisect__helper() helps bypassing
> + * all the code related to finding a commit to test.
> + *
> * If no_checkout is non-zero, the bisection process does not
> * checkout the trial commit but instead simply updates BISECT_HEAD.
> */
Not a problem introduced by this step, but the above description on
no_checkout describes a parameter that no longer exists.
The comments before a function is to guide the developers how to
call the function correctly, so it should have been removed, moved
to where no_checkout is used in the function, or moved to where
BISECT_HEAD ref gets created, as necessary, but by mistake be5fe200
(cmd_bisect__helper: defer parsing no-checkout flag, 2020-08-07),
forgot to do any of the three.
> +static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
> +{
> + int no_checkout;
> + enum bisect_error res;
> +
> + bisect_autostart(terms);
> + if (bisect_next_check(terms, terms->term_good))
> + return BISECT_FAILED;
> +
> + no_checkout = ref_exists("BISECT_HEAD");
> +
> + /* Perform all bisection computation */
> + res = bisect_next_all(the_repository, prefix);
> +
> + if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
> + res = bisect_successful(terms);
> + return res ? res : BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND;
> + } else if (res == BISECT_ONLY_SKIPPED_LEFT) {
> + res = bisect_skipped_commits(terms);
> + return res ? res : BISECT_ONLY_SKIPPED_LEFT;
> + }
> + return res;
> +}
> +
The no_checkout local variable is assigned but never used. It is
understandable if a variable that used to be used becomes unused
when some part (i.e. the part that used to use the variable) of a
function is factored out, but it is rather unusual how a brand new
function has such an unused code and stay to be that way throughout
a topic. Makes a reviewer suspect that there may be a code missing,
that has to use the variable to decide to do things differently, in
this function. It seems to break -Werror builds.
Thanks.
^ permalink raw reply
* Re: [PATCH] Documentation/git-send-email.txt: Mention less secure app access might need to enable.
From: Junio C Hamano @ 2020-08-29 19:29 UTC (permalink / raw)
To: Vasyl Vavrychuk; +Cc: Eric Sunshine, Git List
In-Reply-To: <xmqqwo1hi9nv.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
> Vasyl Vavrychuk <vvavrychuk@gmail.com> writes:
Another thing I forgot to say.
Subject: [PATCH] Documentation/git-send-email.txt: Mention less secure app access might need to enable.
Especially with grammofix s/to enable/to be enabled/ applied, the
above is way too long as a title and would stand out like a sore
thumb in "git shortlog --no-merges v2.28.0..v2.29.0" output.
Something like
Subject: [PATCH] send-email doc: mention less secure app access with GMail
perhaps.
Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Aug 2020, #07; Thu, 27)
From: Junio C Hamano @ 2020-08-29 18:59 UTC (permalink / raw)
To: Hariom verma; +Cc: git, Eric Sunshine, Christian Couder, Heba Waly
In-Reply-To: <CA+CkUQ_3jvkwNvakGxCvDk-C2RMCfAd7pAxy8OmjwVvJT2S_Cw@mail.gmail.com>
Hariom verma <hariom18599@gmail.com> writes:
> Yeah, I agree with you.
> Let's not delay these 2 patches. Sorry for the noise though.
Please do not be sorry, as you didn't raise any unnecessary noise.
Sanity checking each others' action is a good way to make sure we
collectively avoid mistakes, and you just did with the other topic
of yours yesterday, which was very appreciated.
Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Aug 2020, #07; Thu, 27)
From: Hariom verma @ 2020-08-29 13:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqo8muo5nt.fsf@gitster.c.googlers.com>
Hi,
On Sat, Aug 29, 2020 at 2:21 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Hariom verma <hariom18599@gmail.com> writes:
>
> >> * hv/ref-filter-misc (2020-08-17) 9 commits
> >> (merged to 'next' on 2020-08-27 at c015fa6b0f)
> >> + ref-filter: add `sanitize` option for 'subject' atom
> >> + format-support: move `format_sanitized_subject()` from pretty
> >> + pretty: refactor `format_sanitized_subject()`
> >> + ref-filter: add `short` modifier to 'parent' atom
> >> + ref-filter: add `short` modifier to 'tree' atom
> >> + ref-filter: rename `objectname` related functions and fields
> >> + ref-filter: modify error messages in `grab_objectname()`
> >> + ref-filter: refactor `grab_objectname()`
> >> + ref-filter: support different email formats
> >>
> >> The "--format=" option to the "for-each-ref" command and friends
> >> learned a few more tricks, e.g. the ":short" suffix that applies to
> >> "objectname" now also can be used for "parent", "tree", etc.
> >>
> >> Will merge to 'master'.
> >
> > I sent an updated version of the patch series addressing your comment
> > concerning new file format-support.{c,h}[1].
>
> But other stuff (like format-sanitized-subject having unnecessary
> allocation and unnecessary special casing of LF) are worth fixing in
> the version queued above.
Yeah, I fixed that too in the v4 of the patch series.
> Let's revert the above out of 'next' and start afresh using v4.
Just saw v4 in the `seen` ;)
Thanks,
Hariom
^ permalink raw reply
* Re: [PATCH v2 2/2] wt-status: tolerate dangling marks
From: Junio C Hamano @ 2020-08-29 18:55 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, jrnieder
In-Reply-To: <59b91a206d9d7bf64825cb48c747730e28b10a79.1598662525.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> +
> + /*
> + * If ^{upstream} or ^{push} (or equivalent) is requested, and the
> + * branch in question does not have such a reference, return -1 instead
> + * of die()-ing.
> + */
> + unsigned nonfatal_dangling_mark : 1;
Micronit; I would have avoided "or equivalent" as the point of
parenthetical comment was not to say these two modifiers upstream
and push (and other forms that spell differently but invokes exactly
one of these two features) are special, but to say that these two
are merely examples, and any other ^{modifiers} we have or we will
add in the future would honor this bit. Perhaps "(and the like)"?
> };
> int repo_interpret_branch_name(struct repository *r,
> const char *str, int len,
> diff --git a/refs.c b/refs.c
> index cf09cd039f..b6f1a2f452 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -598,10 +598,13 @@ const char *git_default_branch_name(void)
> * to name a branch.
> */
> static char *substitute_branch_name(struct repository *r,
> - const char **string, int *len)
> + const char **string, int *len,
> + int nonfatal_dangling_mark)
> {
> struct strbuf buf = STRBUF_INIT;
> - struct interpret_branch_name_options options = { 0 } ;
> + struct interpret_branch_name_options options = {
> + .nonfatal_dangling_mark = nonfatal_dangling_mark
> + };
> int ret = repo_interpret_branch_name(r, *string, *len, &buf, &options);
>
> if (ret == *len) {
> @@ -615,9 +618,10 @@ static char *substitute_branch_name(struct repository *r,
> }
>
> int repo_dwim_ref(struct repository *r, const char *str, int len,
> - struct object_id *oid, char **ref)
> + struct object_id *oid, char **ref, int nonfatal_dangling_mark)
> {
> - char *last_branch = substitute_branch_name(r, &str, &len);
> + char *last_branch = substitute_branch_name(r, &str, &len,
> + nonfatal_dangling_mark);
> int refs_found = expand_ref(r, str, len, oid, ref);
> free(last_branch);
> return refs_found;
> @@ -625,7 +629,7 @@ int repo_dwim_ref(struct repository *r, const char *str, int len,
>
> int dwim_ref(const char *str, int len, struct object_id *oid, char **ref)
> {
> - return repo_dwim_ref(the_repository, str, len, oid, ref);
> + return repo_dwim_ref(the_repository, str, len, oid, ref, 0);
> }
>
> int expand_ref(struct repository *repo, const char *str, int len,
> @@ -666,7 +670,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
> struct object_id *oid, char **log)
> {
> struct ref_store *refs = get_main_ref_store(r);
> - char *last_branch = substitute_branch_name(r, &str, &len);
> + char *last_branch = substitute_branch_name(r, &str, &len, 0);
> const char **p;
> int logs_found = 0;
> struct strbuf path = STRBUF_INIT;
Among these callers that reach substitute_branch_name(), how were
those that can specify the new bit chosen?
For example, what is the reasoning behind making dwim_ref() unable
to ask the "do so gently" variant, while allowing repo_dwim_ref()
to?
I am NOT necessarily saying these two functions MUST be able to
access the same set of features and the only difference between them
MUST be kept to the current "repo_* variant can work on an arbitrary
repository, while the variant without repo_* would work on the
primary repository only". As long as there is a good reason to make
their power diverge, it is OK---I just do not see why and I'd like
to know.
The same question about not allowing the gentler variant while
drimming the reflog.
Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Aug 2020, #07; Thu, 27)
From: Hariom verma @ 2020-08-29 13:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine, Christian Couder, Heba Waly
In-Reply-To: <xmqq1rjpjohy.fsf@gitster.c.googlers.com>
Hi,
On Sat, Aug 29, 2020 at 11:58 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Hariom verma <hariom18599@gmail.com> writes:
>
> > On Fri, Aug 28, 2020 at 3:14 AM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> [Cooking]
> >>
> >> * hv/ref-filter-trailers-atom-parsing-fix (2020-08-21) 2 commits
> >> (merged to 'next' on 2020-08-24 at 79b27f3263)
> >> + ref-filter: 'contents:trailers' show error if `:` is missing
> >> + t6300: unify %(trailers) and %(contents:trailers) tests
> >> ...
> >
> > After a discussion, we agreed on keeping the helper function instead
> > on duplicating code in "ref-filter: 'contents:trailers' show error if
> > `:` is missing"
> > There is a high possibility we might want to reuse helper in other places too.
> > Especially in the case of newly added "%(subject:sanitize)", if we
> > also want "%(contents:subject:sanitize)" to work.
> >
> > Full discussion:
> > https://public-inbox.org/git/CA+CkUQ8Gst2RTaXY6t+ytWu_9Pu7eqnRYRrnawRwYd_NN=u0Lg@mail.gmail.com/
> >
> > I'm about to send the updated patch series soon.
>
> IIRC, the code in question is good for the purpose of what already
> exists and is easier to follow without helper. When we need to make
> the code more elaborabed and/or when we actually have the second
> callsite would be the ideal time to introduce such a helper as a
> preliminay clean-up patch early in such a follow-on series that
> would happen after the "fix" in question graduates, I would think.
>
> To be honest, I am not sure if we even need an incremental on top
> right now, unless we want to delay the two patches to fix real
> breakage above and make them wait for patches that adds features
> that require to call the same helper from elsewhere.
>
Yeah, I agree with you.
Let's not delay these 2 patches. Sorry for the noise though.
Thanks,
Hariom
^ permalink raw reply
* Re: [PATCH v2 1/2] sha1-name: replace unsigned int with option struct
From: Junio C Hamano @ 2020-08-29 18:44 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, jrnieder
In-Reply-To: <1ce44900a08857332ee70b916c3d9e7e76751221.1598662525.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> In preparation for a future patch adding a boolean parameter to
> repo_interpret_branch_name(), which might be easily confused with an
> existing unsigned int parameter, refactor repo_interpret_branch_name()
> to take an option struct instead of the unsigned int parameter.
Makes sense.
> #define INTERPRET_BRANCH_LOCAL (1<<0)
> #define INTERPRET_BRANCH_REMOTE (1<<1)
> #define INTERPRET_BRANCH_HEAD (1<<2)
> +struct interpret_branch_name_options {
> + /*
> + * If "allowed" is non-zero, it is a treated as a bitfield of allowable
> + * expansions: local branches ("refs/heads/"), remote branches
> + * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
> + * allowed, even ones to refs outside of those namespaces.
> + */
> + unsigned allowed;
> +};
> int repo_interpret_branch_name(struct repository *r,
> const char *str, int len,
> struct strbuf *buf,
> - unsigned allowed);
> -#define interpret_branch_name(str, len, buf, allowed) \
> - repo_interpret_branch_name(the_repository, str, len, buf, allowed)
> + const struct interpret_branch_name_options *options);
> +#define interpret_branch_name(str, len, buf, options) \
> + repo_interpret_branch_name(the_repository, str, len, buf, options)
I was debating myself if we want to have
#define IBN_OPTIONS_INIT { 0 }
or something similar (perhaps "#define IOI(abit) { .allowed = (abit) }"),
but it probably is not worth it given that we have only 3 local
sites that define it, 1 always initializes the field to 0, and the
other just relay the value passed by its caller.
> ...
> diff --git a/sha1-name.c b/sha1-name.c
> index 0b8cb5247a..a7a9de66c4 100644
> --- a/sha1-name.c
> +++ b/sha1-name.c
> @@ -1427,9 +1427,12 @@ static int reinterpret(struct repository *r,
> struct strbuf tmp = STRBUF_INIT;
> int used = buf->len;
> int ret;
> + struct interpret_branch_name_options options = {
> + .allowed = allowed
> + };
>
> strbuf_add(buf, name + len, namelen - len);
> - ret = repo_interpret_branch_name(r, buf->buf, buf->len, &tmp, allowed);
> + ret = repo_interpret_branch_name(r, buf->buf, buf->len, &tmp, &options);
> @@ -1557,7 +1561,10 @@ int repo_interpret_branch_name(struct repository *r,
> void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
> {
> int len = strlen(name);
> - int used = interpret_branch_name(name, len, sb, allowed);
> + struct interpret_branch_name_options options = {
> + .allowed = allowed
> + };
> + int used = interpret_branch_name(name, len, sb, &options);
These are quite straight-forward rewrites. Looking good.
Thanks.
^ permalink raw reply
* Re: [PATCH] Documentation/git-send-email.txt: Mention less secure app access might need to enable.
From: Junio C Hamano @ 2020-08-29 18:33 UTC (permalink / raw)
To: Vasyl Vavrychuk; +Cc: Eric Sunshine, Git List
In-Reply-To: <20200829153920.17155-1-vvavrychuk@gmail.com>
Vasyl Vavrychuk <vvavrychuk@gmail.com> writes:
> Looks like Google changed Gmail security and now less secure app access
> needs to be explicitly enabled if 2-factor authentication is not in
> place, otherwise send-mail fails with:
'send-email' I presume?
> 5.7.8 Username and Password not accepted. Learn more at
> 5.7.8 https://support.google.com/mail/?p=BadCredentials v5sm13756502ede.13 - gsmtp
Is the v5sm1375... part stay constant that everybody would get the
same thing?
Otherwise,
... otherwise send-email fails with an error message from Gmail
server telling the user to "Learn more" at
https://support.google.com/mail/?p=BadCredentials
would be preferrable.
> Signed-off-by: Vasyl Vavrychuk <vvavrychuk@gmail.com>
> ---
> Documentation/git-send-email.txt | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 0a69810147..06953fd1e0 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -494,7 +494,11 @@ edit ~/.gitconfig to specify your account settings:
> smtpServerPort = 587
> ----
>
> -If you have multifactor authentication setup on your gmail account, you will
> +If you do not have multi-factor authentication set up on your Gmail account, you
> +will need to allow less secure app access. Visit
> +https://myaccount.google.com/lesssecureapps to enable it.
> +
> +If you have multi-factor authentication set up on your Gmail account, you will
> need to generate an app-specific password for use with 'git send-email'. Visit
> https://security.google.com/settings/security/apppasswords to create it.
In 2020, we should probably list the instruction for those with 2
factor first and then give "less secure app access" as a fallback.
IOW, I'd suggest the order of these two paragraphs swapped.
Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Aug 2020, #07; Thu, 27)
From: Junio C Hamano @ 2020-08-29 18:28 UTC (permalink / raw)
To: Hariom verma; +Cc: git, Eric Sunshine, Christian Couder, Heba Waly
In-Reply-To: <CA+CkUQ-SsxrJk+7e-ygm8FfCto6XZt2Ts9UcTMpgkmAQWZkLhA@mail.gmail.com>
Hariom verma <hariom18599@gmail.com> writes:
> On Fri, Aug 28, 2020 at 3:14 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> [Cooking]
>>
>> * hv/ref-filter-trailers-atom-parsing-fix (2020-08-21) 2 commits
>> (merged to 'next' on 2020-08-24 at 79b27f3263)
>> + ref-filter: 'contents:trailers' show error if `:` is missing
>> + t6300: unify %(trailers) and %(contents:trailers) tests
>> ...
>
> After a discussion, we agreed on keeping the helper function instead
> on duplicating code in "ref-filter: 'contents:trailers' show error if
> `:` is missing"
> There is a high possibility we might want to reuse helper in other places too.
> Especially in the case of newly added "%(subject:sanitize)", if we
> also want "%(contents:subject:sanitize)" to work.
>
> Full discussion:
> https://public-inbox.org/git/CA+CkUQ8Gst2RTaXY6t+ytWu_9Pu7eqnRYRrnawRwYd_NN=u0Lg@mail.gmail.com/
>
> I'm about to send the updated patch series soon.
Days after the topic got merged to 'next' is not a time to send any
updated patch series. It should come in the form of incremental
update on top of what is already there.
IIRC, the code in question is good for the purpose of what already
exists and is easier to follow without helper. When we need to make
the code more elaborabed and/or when we actually have the second
callsite would be the ideal time to introduce such a helper as a
preliminay clean-up patch early in such a follow-on series that
would happen after the "fix" in question graduates, I would think.
To be honest, I am not sure if we even need an incremental on top
right now, unless we want to delay the two patches to fix real
breakage above and make them wait for patches that adds features
that require to call the same helper from elsewhere.
Thanks.
^ permalink raw reply
* [PATCH] Documentation/git-send-email.txt: Mention less secure app access might need to enable.
From: Vasyl Vavrychuk @ 2020-08-29 15:39 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Vasyl Vavrychuk
In-Reply-To: <CAPig+cT8Kmh6LcoKqkcJX6imXaase07o8C_-7k7RkyhEyW02rQ@mail.gmail.com>
Looks like Google changed Gmail security and now less secure app access
needs to be explicitly enabled if 2-factor authentication is not in
place, otherwise send-mail fails with:
5.7.8 Username and Password not accepted. Learn more at
5.7.8 https://support.google.com/mail/?p=BadCredentials v5sm13756502ede.13 - gsmtp
Signed-off-by: Vasyl Vavrychuk <vvavrychuk@gmail.com>
---
Documentation/git-send-email.txt | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 0a69810147..06953fd1e0 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -494,7 +494,11 @@ edit ~/.gitconfig to specify your account settings:
smtpServerPort = 587
----
-If you have multifactor authentication setup on your gmail account, you will
+If you do not have multi-factor authentication set up on your Gmail account, you
+will need to allow less secure app access. Visit
+https://myaccount.google.com/lesssecureapps to enable it.
+
+If you have multi-factor authentication set up on your Gmail account, you will
need to generate an app-specific password for use with 'git send-email'. Visit
https://security.google.com/settings/security/apppasswords to create it.
--
2.23.0
^ permalink raw reply related
* Re: What's cooking in git.git (Aug 2020, #07; Thu, 27)
From: Hariom verma @ 2020-08-29 10:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine, Christian Couder, Heba Waly
In-Reply-To: <xmqqh7snpxy1.fsf@gitster.c.googlers.com>
Hi,
On Fri, Aug 28, 2020 at 3:14 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> [Cooking]
>
> * hv/ref-filter-trailers-atom-parsing-fix (2020-08-21) 2 commits
> (merged to 'next' on 2020-08-24 at 79b27f3263)
> + ref-filter: 'contents:trailers' show error if `:` is missing
> + t6300: unify %(trailers) and %(contents:trailers) tests
>
> The parser for "git for-each-ref --format=..." was too loose when
> parsing the "%(trailers...)" atom, and forgot that "trailers" and
> "trailers:<modifers>" are the only two allowed forms, which has
> been corrected.
>
> Will merge to 'master'.
>
After a discussion, we agreed on keeping the helper function instead
on duplicating code in "ref-filter: 'contents:trailers' show error if
`:` is missing"
There is a high possibility we might want to reuse helper in other places too.
Especially in the case of newly added "%(subject:sanitize)", if we
also want "%(contents:subject:sanitize)" to work.
Full discussion:
https://public-inbox.org/git/CA+CkUQ8Gst2RTaXY6t+ytWu_9Pu7eqnRYRrnawRwYd_NN=u0Lg@mail.gmail.com/
I'm about to send the updated patch series soon.
Thanks,
Hariom
^ permalink raw reply
* Re: Fastest way to set files date and time to latest commit time of each one
From: Andreas Schwab @ 2020-08-29 6:46 UTC (permalink / raw)
To: Ivan Baldo; +Cc: git
In-Reply-To: <CAEbcw=3mOoYuJo2mQgqB2aJgn-D2i_7ZRmhfPvYNVHD1Kp8wuA@mail.gmail.com>
I'm using this script:
#!/bin/sh
git log --name-only --format=format:%n%ct -- "$@" |
perl -e 'my $do_date = 0; chomp(my $cdup = `git rev-parse --show-cdup`);
while (<>) {
chomp;
if ($do_date) {
next if ($_ eq "");
die "Unexpected $_\n" unless /^[0-9]+$/;
$d = $_;
$do_date = 0;
} elsif ($_ eq "") {
$do_date = 1;
} elsif (!defined($seen{$_})) {
$seen{$_} = 1;
utime $d, $d, "$cdup$_";
}
}'
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply
* Re: Fastest way to set files date and time to latest commit time of each one
From: Raymond E. Pasco @ 2020-08-29 4:59 UTC (permalink / raw)
To: Junio C Hamano, Ivan Baldo; +Cc: git
In-Reply-To: <xmqq8sdym93d.fsf@gitster.c.googlers.com>
On Fri Aug 28, 2020 at 11:20 PM EDT, Junio C Hamano wrote:
> - The source of the rsync transfer is a git working tree. It often
> has the checkout of the latest and greatest version, but during
> development, it may switch to older commit (e.g. to find where
> regression occurred) or not-yet-ready commit (e.g. work in
> progress that is not given to upstream). You check out the
> version you want to sync to the destination before initiating
> rsync.
Assuming this is the case, perhaps a separate worktree that you touch
less often being the source for rsync might save rsync some
bandwidth/cpu checking things.
^ permalink raw reply
* Re: Fastest way to set files date and time to latest commit time of each one
From: Eric Wong @ 2020-08-29 4:48 UTC (permalink / raw)
To: Ivan Baldo; +Cc: git
In-Reply-To: <CAEbcw=3mOoYuJo2mQgqB2aJgn-D2i_7ZRmhfPvYNVHD1Kp8wuA@mail.gmail.com>
Ivan Baldo <ibaldo@gmail.com> wrote:
> Hello.
> I know this is not standard usage of git, but I need a way to have
> more stable dates and times in the files in order to avoid rsync
> checksumming.
> So I found this
> https://stackoverflow.com/questions/2179722/checking-out-old-file-with-original-create-modified-timestamps/2179876#2179876
> and modified it a bit to run in CentOS 7:
>
> IFS="
> "
> for FILE in $(git ls-files -z | tr '\0' '\n')
> do
> TIME=$(git log --pretty=format:%cd -n 1 --date=iso -- "$FILE")
> touch -c -m -d "$TIME" "$FILE"
> done
>
> Unfortunately it takes ages for a 84k files repo.
> I see the CPU usage is dominated by the git log command.
running git log for each file isn't necessary.
On Debian, rsync actually ships the `git-set-file-times' script
in /usr/share/doc/rsync/scripts/ which only runs `git log' once
and parses it.
You can also get my (original) version from:
https://yhbt.net/git-set-file-times
> I know a way I could use to split the work for all the CPU threads
> but anyway, I would like to know if you guys and girls know of a
> faster way to do this.
Much of your overhead is going to be from process spawning.
My Perl version reduces that significantly.
I haven't tried it with 84K files, but it'll have to keep all
those filenames in memory. I'm not sure if parallelizing
utime() syscalls is worth it, either; maybe it helps on SSD
more than HDD.
^ permalink raw reply
* Re: What's cooking in git.git (Aug 2020, #07; Thu, 27)
From: Matheus Tavares Bernardino @ 2020-08-29 4:28 UTC (permalink / raw)
To: Elijah Newren; +Cc: Jonathan Nieder, Git Mailing List, Junio C Hamano
In-Reply-To: <CABPp-BHCVWWFDL2kpWymNuX_t4anX2Xw-xiTK8N+MdrBoopERg@mail.gmail.com>
Hi, Elijah
On Thu, Aug 27, 2020 at 11:16 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Thu, Aug 27, 2020 at 2:46 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > * mt/grep-sparse-checkout (2020-06-12) 6 commits
> > - config: add setting to ignore sparsity patterns in some cmds
> > - grep: honor sparse checkout patterns
> > - config: correctly read worktree configs in submodules
> > - t/helper/test-config: facilitate addition of new cli options
> > - t/helper/test-config: return exit codes consistently
> > - doc: grep: unify info on configuration variables
> >
> > "git grep" has been tweaked to be limited to the sparse checkout
> > paths.
> >
> > Review needed on 4/6; otherwise looking sane.
> > cf. <CABPp-BGdEyEeajYZj_rdxp=MyEQdszuyjVTax=hhYj3fOtRQUQ@mail.gmail.com>
>
> Kinda disappointed to see this stalled out; especially after so much
> work put into it. This spawned lots of other side discussions and a
> topic or two outside this series as well. I really like the overall
> result we came up with out of this series and would like to see it
> land [...]
Yeah, I would also really like to see it land :) Thanks to your last
rounds of review, I think we got to a much better design regarding
each grep case.
> [...] If we can't get more reviewers, maybe we just merge it down
> anyway? But, in an attempt to prod some review...
>
> Jonathan: I pinged you in chat about this series some time ago and you
> said you'd take a look and comment. I can't find a reference
> anywhere, but maybe you talked with Matheus in IRC somewhere? Do you
> recall?
>
> Matheus: Do you have any outstanding items for this series or is it
> good as far as you know?
Hm, I don't have anything else planned to add.
Just one minor aesthetic consideration: should we perhaps eliminate
the sparse-checkout.c file (and the accompanying header) from the last
patch? This file contains only one function; It was created with the
idea of being later populated by another parallel series [1]. However,
if we only have this function for now, should we move it elsewhere?
I'm not sure where, though. Maybe to config.c, together with
git_config_get_index_threads() and others?
[1]: https://lore.kernel.org/git/2188577cd848d7cee77f06f1ad2b181864e5e36d.1588857462.git.gitgitgadget@gmail.com/
^ permalink raw reply
* Re: Fastest way to set files date and time to latest commit time of each one
From: Junio C Hamano @ 2020-08-29 3:20 UTC (permalink / raw)
To: Ivan Baldo; +Cc: git
In-Reply-To: <CAEbcw=3mOoYuJo2mQgqB2aJgn-D2i_7ZRmhfPvYNVHD1Kp8wuA@mail.gmail.com>
Ivan Baldo <ibaldo@gmail.com> writes:
> I know this is not standard usage of git, but I need a way to have
> more stable dates and times in the files in order to avoid rsync
> checksumming.
Would you care to elaborate a bit more about the use case? From
what you wrote, I would assume:
- The source of the rsync transfer is a git working tree. It often
has the checkout of the latest and greatest version, but during
development, it may switch to older commit (e.g. to find where
regression occurred) or not-yet-ready commit (e.g. work in
progress that is not given to upstream). You check out the
version you want to sync to the destination before initiating
rsync.
- The destination of the rsync transfer is meant to serve as a
back-up of the latest and greatest, periodical snapshot of a
branch, etc., which is NOT controlled by git and transfer does
not happen in the reverse direction [*2*]
Because the working tree of the source repository is used to check
out different versions between rsync sessions, files that did not
change between the commit you sync'ed to destination the last time
and the commit you are about to sync still may have been touched and
have different timestamp, requiring rsync to check the contents.
And as a workaround, you are willing to change the workflow to
"touch" the working tree files, immediately before you run the next
rsync, in a predictable way so that the timestamp of a file whose
contents did not change since the last rsync session would have the
same timestamp. This may break your build next time you run "make"
in the source working tree (because your object files that are
excluded from your rsync may have newer timestamp than the
corresponding source even when they must be recompiled due to your
"touch"ing), but you are willing to pay the cost of say "make clean"
after "touch"ing.
Is that the kind of use case you have around "rsync"?
To the question "what is the time this file was last modified?",
there is no simple and cheap answer that is easy to explain to
end-users, unless your development is completely linear [*2*].
The loop you showed would be the right one in a linear history, and
with recent development to record which paths were changed in each
commit in the commit-graph data structure, the script should work a
lot faster than traditional git.
[Footnote]
*1* Otherwise, you'd be just mirror-fetching from the source
repository. If that can be arranged, running "git pull --ff-only"
on the destination side to update from the source side would
be a lot more efficient than running rsync, I would imagine.
*2* In a history with merges, because two or more branches can touch
the file in parallel development at different times and then the
resulting parallel histories get merged into a single history.
When two or more of these parallel history gave the file in
question an identical content at different times, and the merge
result was recorded as the same content, you'd need to follow
ALL the paths and compare the timestamp of these commits to pick
one (which one? the oldest one? the newest one? does the
order of parents in the merge matter?)
^ permalink raw reply
* Fastest way to set files date and time to latest commit time of each one
From: Ivan Baldo @ 2020-08-29 1:36 UTC (permalink / raw)
To: git
Hello.
I know this is not standard usage of git, but I need a way to have
more stable dates and times in the files in order to avoid rsync
checksumming.
So I found this
https://stackoverflow.com/questions/2179722/checking-out-old-file-with-original-create-modified-timestamps/2179876#2179876
and modified it a bit to run in CentOS 7:
IFS="
"
for FILE in $(git ls-files -z | tr '\0' '\n')
do
TIME=$(git log --pretty=format:%cd -n 1 --date=iso -- "$FILE")
touch -c -m -d "$TIME" "$FILE"
done
Unfortunately it takes ages for a 84k files repo.
I see the CPU usage is dominated by the git log command.
I know a way I could use to split the work for all the CPU threads
but anyway, I would like to know if you guys and girls know of a
faster way to do this.
Also I know of other utilities that store the metadata in Git, but I
am trying to avoid that for the moment.
Thanks a lot in advance!
Have a nice day.
P.s.: please Cc replies to me.
--
Ivan Baldo - ibaldo@gmail.com - http://ibaldo.codigolibre.net/
Freelance C++/PHP programmer and GNU/Linux systems administrator.
The sky isn't the limit!
^ permalink raw reply
* [PATCH v2 2/2] wt-status: tolerate dangling marks
From: Jonathan Tan @ 2020-08-29 1:02 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, jrnieder
In-Reply-To: <cover.1598662525.git.jonathantanmy@google.com>
When a user checks out the upstream branch of HEAD, the upstream branch
not being a local branch, and then runs "git status", like this:
git clone $URL client
cd client
git checkout @{u}
git status
no status is printed, but instead an error message:
fatal: HEAD does not point to a branch
(This error message when running "git branch" persists even after
checking out other things - it only stops after checking out a branch.)
This is because "git status" reads the reflog when determining the "HEAD
detached" message, and thus attempts to DWIM "@{u}", but that doesn't
work because HEAD no longer points to a branch.
Therefore, when calculating the status of a worktree, tolerate dangling
marks. This is done by adding an additional parameter to
repo_dwim_ref().
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
cache.h | 7 +++++++
refs.c | 16 ++++++++++------
refs.h | 3 ++-
sha1-name.c | 16 +++++++++++-----
t/t7508-status.sh | 12 ++++++++++++
wt-status.c | 2 +-
6 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/cache.h b/cache.h
index 4f16a57ba4..cee8aa5dc3 100644
--- a/cache.h
+++ b/cache.h
@@ -1569,6 +1569,13 @@ struct interpret_branch_name_options {
* allowed, even ones to refs outside of those namespaces.
*/
unsigned allowed;
+
+ /*
+ * If ^{upstream} or ^{push} (or equivalent) is requested, and the
+ * branch in question does not have such a reference, return -1 instead
+ * of die()-ing.
+ */
+ unsigned nonfatal_dangling_mark : 1;
};
int repo_interpret_branch_name(struct repository *r,
const char *str, int len,
diff --git a/refs.c b/refs.c
index cf09cd039f..b6f1a2f452 100644
--- a/refs.c
+++ b/refs.c
@@ -598,10 +598,13 @@ const char *git_default_branch_name(void)
* to name a branch.
*/
static char *substitute_branch_name(struct repository *r,
- const char **string, int *len)
+ const char **string, int *len,
+ int nonfatal_dangling_mark)
{
struct strbuf buf = STRBUF_INIT;
- struct interpret_branch_name_options options = { 0 } ;
+ struct interpret_branch_name_options options = {
+ .nonfatal_dangling_mark = nonfatal_dangling_mark
+ };
int ret = repo_interpret_branch_name(r, *string, *len, &buf, &options);
if (ret == *len) {
@@ -615,9 +618,10 @@ static char *substitute_branch_name(struct repository *r,
}
int repo_dwim_ref(struct repository *r, const char *str, int len,
- struct object_id *oid, char **ref)
+ struct object_id *oid, char **ref, int nonfatal_dangling_mark)
{
- char *last_branch = substitute_branch_name(r, &str, &len);
+ char *last_branch = substitute_branch_name(r, &str, &len,
+ nonfatal_dangling_mark);
int refs_found = expand_ref(r, str, len, oid, ref);
free(last_branch);
return refs_found;
@@ -625,7 +629,7 @@ int repo_dwim_ref(struct repository *r, const char *str, int len,
int dwim_ref(const char *str, int len, struct object_id *oid, char **ref)
{
- return repo_dwim_ref(the_repository, str, len, oid, ref);
+ return repo_dwim_ref(the_repository, str, len, oid, ref, 0);
}
int expand_ref(struct repository *repo, const char *str, int len,
@@ -666,7 +670,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
struct object_id *oid, char **log)
{
struct ref_store *refs = get_main_ref_store(r);
- char *last_branch = substitute_branch_name(r, &str, &len);
+ char *last_branch = substitute_branch_name(r, &str, &len, 0);
const char **p;
int logs_found = 0;
struct strbuf path = STRBUF_INIT;
diff --git a/refs.h b/refs.h
index 29e28124cd..b94a7fd4f7 100644
--- a/refs.h
+++ b/refs.h
@@ -149,7 +149,8 @@ struct strvec;
void expand_ref_prefix(struct strvec *prefixes, const char *prefix);
int expand_ref(struct repository *r, const char *str, int len, struct object_id *oid, char **ref);
-int repo_dwim_ref(struct repository *r, const char *str, int len, struct object_id *oid, char **ref);
+int repo_dwim_ref(struct repository *r, const char *str, int len,
+ struct object_id *oid, char **ref, int nonfatal_dangling_mark);
int repo_dwim_log(struct repository *r, const char *str, int len, struct object_id *oid, char **ref);
int dwim_ref(const char *str, int len, struct object_id *oid, char **ref);
int dwim_log(const char *str, int len, struct object_id *oid, char **ref);
diff --git a/sha1-name.c b/sha1-name.c
index a7a9de66c4..0b23b86ceb 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -809,7 +809,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
if (len == r->hash_algo->hexsz && !get_oid_hex(str, oid)) {
if (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {
- refs_found = repo_dwim_ref(r, str, len, &tmp_oid, &real_ref);
+ refs_found = repo_dwim_ref(r, str, len, &tmp_oid, &real_ref, 0);
if (refs_found > 0) {
warning(warn_msg, len, str);
if (advice_object_name_warning)
@@ -860,11 +860,11 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
if (!len && reflog_len)
/* allow "@{...}" to mean the current branch reflog */
- refs_found = repo_dwim_ref(r, "HEAD", 4, oid, &real_ref);
+ refs_found = repo_dwim_ref(r, "HEAD", 4, oid, &real_ref, 0);
else if (reflog_len)
refs_found = repo_dwim_log(r, str, len, oid, &real_ref);
else
- refs_found = repo_dwim_ref(r, str, len, oid, &real_ref);
+ refs_found = repo_dwim_ref(r, str, len, oid, &real_ref, 0);
if (!refs_found)
return -1;
@@ -1496,8 +1496,14 @@ static int interpret_branch_mark(struct repository *r,
branch = branch_get(NULL);
value = get_data(branch, &err);
- if (!value)
- die("%s", err.buf);
+ if (!value) {
+ if (options->nonfatal_dangling_mark) {
+ strbuf_release(&err);
+ return -1;
+ } else {
+ die("%s", err.buf);
+ }
+ }
if (!branch_interpret_allowed(value, options->allowed))
return -1;
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index e81759319f..45e1f6ff68 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -846,6 +846,18 @@ test_expect_success 'status refreshes the index' '
test_cmp expect output
'
+test_expect_success 'status shows detached HEAD properly after checking out non-local upstream branch' '
+ test_when_finished rm -rf upstream downstream actual &&
+
+ test_create_repo upstream &&
+ test_commit -C upstream foo &&
+
+ git clone upstream downstream &&
+ git -C downstream checkout @{u} &&
+ git -C downstream status >actual &&
+ test_i18ngrep "HEAD detached at [0-9a-f]\\+" actual
+'
+
test_expect_success 'setup status submodule summary' '
test_create_repo sm && (
cd sm &&
diff --git a/wt-status.c b/wt-status.c
index 7ce58b8aae..ae16faf40d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1569,7 +1569,7 @@ static void wt_status_get_detached_from(struct repository *r,
return;
}
- if (dwim_ref(cb.buf.buf, cb.buf.len, &oid, &ref) == 1 &&
+ if (repo_dwim_ref(the_repository, cb.buf.buf, cb.buf.len, &oid, &ref, 1) == 1 &&
/* sha1 is a commit? match without further lookup */
(oideq(&cb.noid, &oid) ||
/* perhaps sha1 is a tag, try to dereference to a commit */
--
2.28.0.402.g5ffc5be6b7-goog
^ permalink raw reply related
* [PATCH v2 1/2] sha1-name: replace unsigned int with option struct
From: Jonathan Tan @ 2020-08-29 1:02 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, jrnieder
In-Reply-To: <cover.1598662525.git.jonathantanmy@google.com>
In preparation for a future patch adding a boolean parameter to
repo_interpret_branch_name(), which might be easily confused with an
existing unsigned int parameter, refactor repo_interpret_branch_name()
to take an option struct instead of the unsigned int parameter.
The static function interpret_branch_mark() is also updated to take the
option struct in preparation for that future patch, since it will also
make use of the to-be-introduced boolean parameter.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
cache.h | 20 ++++++++++++--------
refs.c | 3 ++-
revision.c | 3 ++-
sha1-name.c | 29 ++++++++++++++++++-----------
4 files changed, 34 insertions(+), 21 deletions(-)
diff --git a/cache.h b/cache.h
index 4cad61ffa4..4f16a57ba4 100644
--- a/cache.h
+++ b/cache.h
@@ -1557,21 +1557,25 @@ int parse_oid_hex_any(const char *hex, struct object_id *oid, const char **end);
*
* If the input was ok but there are not N branch switches in the
* reflog, it returns 0.
- *
- * If "allowed" is non-zero, it is a treated as a bitfield of allowable
- * expansions: local branches ("refs/heads/"), remote branches
- * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
- * allowed, even ones to refs outside of those namespaces.
*/
#define INTERPRET_BRANCH_LOCAL (1<<0)
#define INTERPRET_BRANCH_REMOTE (1<<1)
#define INTERPRET_BRANCH_HEAD (1<<2)
+struct interpret_branch_name_options {
+ /*
+ * If "allowed" is non-zero, it is a treated as a bitfield of allowable
+ * expansions: local branches ("refs/heads/"), remote branches
+ * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
+ * allowed, even ones to refs outside of those namespaces.
+ */
+ unsigned allowed;
+};
int repo_interpret_branch_name(struct repository *r,
const char *str, int len,
struct strbuf *buf,
- unsigned allowed);
-#define interpret_branch_name(str, len, buf, allowed) \
- repo_interpret_branch_name(the_repository, str, len, buf, allowed)
+ const struct interpret_branch_name_options *options);
+#define interpret_branch_name(str, len, buf, options) \
+ repo_interpret_branch_name(the_repository, str, len, buf, options)
int validate_headref(const char *ref);
diff --git a/refs.c b/refs.c
index 3ee3afaf41..cf09cd039f 100644
--- a/refs.c
+++ b/refs.c
@@ -601,7 +601,8 @@ static char *substitute_branch_name(struct repository *r,
const char **string, int *len)
{
struct strbuf buf = STRBUF_INIT;
- int ret = repo_interpret_branch_name(r, *string, *len, &buf, 0);
+ struct interpret_branch_name_options options = { 0 } ;
+ int ret = repo_interpret_branch_name(r, *string, *len, &buf, &options);
if (ret == *len) {
size_t size;
diff --git a/revision.c b/revision.c
index 96630e3186..1247ee4ec8 100644
--- a/revision.c
+++ b/revision.c
@@ -315,13 +315,14 @@ static void add_pending_object_with_path(struct rev_info *revs,
const char *name, unsigned mode,
const char *path)
{
+ struct interpret_branch_name_options options = { 0 };
if (!obj)
return;
if (revs->no_walk && (obj->flags & UNINTERESTING))
revs->no_walk = 0;
if (revs->reflog_info && obj->type == OBJ_COMMIT) {
struct strbuf buf = STRBUF_INIT;
- int len = interpret_branch_name(name, 0, &buf, 0);
+ int len = interpret_branch_name(name, 0, &buf, &options);
if (0 < len && name[len] && buf.len)
strbuf_addstr(&buf, name + len);
diff --git a/sha1-name.c b/sha1-name.c
index 0b8cb5247a..a7a9de66c4 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -1427,9 +1427,12 @@ static int reinterpret(struct repository *r,
struct strbuf tmp = STRBUF_INIT;
int used = buf->len;
int ret;
+ struct interpret_branch_name_options options = {
+ .allowed = allowed
+ };
strbuf_add(buf, name + len, namelen - len);
- ret = repo_interpret_branch_name(r, buf->buf, buf->len, &tmp, allowed);
+ ret = repo_interpret_branch_name(r, buf->buf, buf->len, &tmp, &options);
/* that data was not interpreted, remove our cruft */
if (ret < 0) {
strbuf_setlen(buf, used);
@@ -1471,7 +1474,7 @@ static int interpret_branch_mark(struct repository *r,
int (*get_mark)(const char *, int),
const char *(*get_data)(struct branch *,
struct strbuf *),
- unsigned allowed)
+ const struct interpret_branch_name_options *options)
{
int len;
struct branch *branch;
@@ -1496,7 +1499,7 @@ static int interpret_branch_mark(struct repository *r,
if (!value)
die("%s", err.buf);
- if (!branch_interpret_allowed(value, allowed))
+ if (!branch_interpret_allowed(value, options->allowed))
return -1;
set_shortened_ref(r, buf, value);
@@ -1506,7 +1509,7 @@ static int interpret_branch_mark(struct repository *r,
int repo_interpret_branch_name(struct repository *r,
const char *name, int namelen,
struct strbuf *buf,
- unsigned allowed)
+ const struct interpret_branch_name_options *options)
{
char *at;
const char *start;
@@ -1515,7 +1518,7 @@ int repo_interpret_branch_name(struct repository *r,
if (!namelen)
namelen = strlen(name);
- if (!allowed || (allowed & INTERPRET_BRANCH_LOCAL)) {
+ if (!options->allowed || (options->allowed & INTERPRET_BRANCH_LOCAL)) {
len = interpret_nth_prior_checkout(r, name, namelen, buf);
if (!len) {
return len; /* syntax Ok, not enough switches */
@@ -1523,7 +1526,8 @@ int repo_interpret_branch_name(struct repository *r,
if (len == namelen)
return len; /* consumed all */
else
- return reinterpret(r, name, namelen, len, buf, allowed);
+ return reinterpret(r, name, namelen, len, buf,
+ options->allowed);
}
}
@@ -1531,22 +1535,22 @@ int repo_interpret_branch_name(struct repository *r,
(at = memchr(start, '@', namelen - (start - name)));
start = at + 1) {
- if (!allowed || (allowed & INTERPRET_BRANCH_HEAD)) {
+ if (!options->allowed || (options->allowed & INTERPRET_BRANCH_HEAD)) {
len = interpret_empty_at(name, namelen, at - name, buf);
if (len > 0)
return reinterpret(r, name, namelen, len, buf,
- allowed);
+ options->allowed);
}
len = interpret_branch_mark(r, name, namelen, at - name, buf,
upstream_mark, branch_get_upstream,
- allowed);
+ options);
if (len > 0)
return len;
len = interpret_branch_mark(r, name, namelen, at - name, buf,
push_mark, branch_get_push,
- allowed);
+ options);
if (len > 0)
return len;
}
@@ -1557,7 +1561,10 @@ int repo_interpret_branch_name(struct repository *r,
void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
{
int len = strlen(name);
- int used = interpret_branch_name(name, len, sb, allowed);
+ struct interpret_branch_name_options options = {
+ .allowed = allowed
+ };
+ int used = interpret_branch_name(name, len, sb, &options);
if (used < 0)
used = 0;
--
2.28.0.402.g5ffc5be6b7-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox