* Re: [PATCH v2 3/2] credential-cache: use child_process.args
From: Jeff King @ 2020-08-27 4:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqzh6ht7fg.fsf_-_@gitster.c.googlers.com>
On Wed, Aug 26, 2020 at 02:37:39PM -0700, Junio C Hamano wrote:
> As child_process structure has an embedded strvec args for
> formulating the command line, let's use it instead of using
> an out-of-line argv[] whose length needs to be maintained
> correctly.
I forgot to mention in my other reply: I think this fails to mention the
actual functional change, which is switching from the dashed form to
using the "git" wrapper.
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/2] credential-cache: use child_process.args
From: Jeff King @ 2020-08-27 4:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqzh6ht7fg.fsf_-_@gitster.c.googlers.com>
On Wed, Aug 26, 2020 at 02:37:39PM -0700, Junio C Hamano wrote:
> diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
> index d0fafdeb9e..195335a783 100644
> --- a/builtin/credential-cache.c
> +++ b/builtin/credential-cache.c
> @@ -42,13 +42,13 @@ static int send_request(const char *socket, const struct strbuf *out)
> static void spawn_daemon(const char *socket)
> {
> struct child_process daemon = CHILD_PROCESS_INIT;
> - const char *argv[] = { NULL, NULL, NULL };
> char buf[128];
> int r;
>
> - argv[0] = "git-credential-cache--daemon";
> - argv[1] = socket;
> - daemon.argv = argv;
> + strvec_pushl(&daemon.args,
> + "credential-cache--daemon", socket,
> + NULL);
> + daemon.git_cmd = 1;
Yep, this makes sense. I don't recall any reason to use the dashed form
back then, but probably it was just that I knew it was a separate
program. Doing it this way will mean an extra parent "git" process
hanging around, but I don't think it's that big a deal. We never try to
kill it by PID, etc (instead we connect to the socket and ask it to
exit). And anyway, it is becoming a builtin in a parallel topic, so that
extra process will go away. :)
-Peff
^ permalink raw reply
* Re: [PATCH] clone: add remote.cloneDefault config option
From: Sean Barag @ 2020-08-27 3:38 UTC (permalink / raw)
To: gitster; +Cc: git, gitgitgadget, sean, stolee
In-Reply-To: <xmqqeentuqk5.fsf@gitster.c.googlers.com>
> Derrick Stolee <stolee@gmail.com> writes:
>
>> On 8/26/2020 2:46 PM, Junio C Hamano wrote:
>>> "Sean Barag via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>> This commit implements
>>>> `remote.cloneDefault` as a parallel to `remote.pushDefault`,
>>>> with prioritized name resolution:
>>>
>>> I highly doubt that .cloneDefault is a good name. After reading
>>> only the title of the patch e-mail, i.e. when the only available
>>> information on the change available to me was the name of the
>>> configuration variable and the fact that it pertains to the command
>>> "git clone", I thought it is to specify a URL, from which "git
>>> clone" without the URL would clone from that single repository.
>>>
>>> And the name will cause the same misunderstanding to normal users,
>>> not just to reviewers of your patch, after this change hits a future
>>> Git release.
>>>
>>> Taking a parallel from init.defaultBranchName, I would probably call
>>> it clone.defaultUpstreamName if I were writing this feature.
>>
>> I was thinking "clone.defaultRemoteName" makes it clear we are naming
>> the remote for the provided <url> in the command.
>
>I 100% agree that defaultremotename is much better.
Perfect, I'll move forward with `clone.defaultRemoteName`. Thanks for
the recommendation. This would be the first config variable inside
the a "clone" subsection -- is there anything special that needs to
happen when a new subsection is added?
>>>> ... For example
>>>
>>> git -c remote.cloneDefault="bad.../...name" clone parent
>>>
>>> should fail, no?
>>
>> This is an important suggestion.
>
> To be fair, the current code does not handle the "--origin" command
> line option not so carefully.
Agreed - I'm sorry for not including those tests. They'll be present
in v2. I'll be sure to include some validation for
`clone.defaultRemoteName` within `git_config` as well.
> It is somewhat sad that we have the git_config(git_default_config)
> call so late in the control flow. I wonder if we can update the
> start-up sequence to match the usual flow
> ...
> One oddity "git clone" has is that it wants to delay the reading of
> configuration files (they are read only once, and second and
> subsequent git_config() calls will reuse what was read before [*]) so
> that it can read what clone.c::write_config() wrote, so if we were to
> "fix" the start-up sequence to match the usual flow, we need to
> satisfy what that odd arrangement wanted to achieve in some other way
> (e.g. feed what is in option_config to git_default_config ourselves,
> without using git_config(), as part of the "main control flow uses the
> variable" part), but it should be doable.
Sounds like a pretty big change! I'm willing to take a crack at it,
but given that this is my first patch I'm frankly a bit intimidated :)
How would you feel about that being a separate patch?
Thanks for all the guidance, folks.
Sean
^ permalink raw reply
* Re: [PATCH] wt-status: expand, not dwim, a "detached from" ref
From: Junio C Hamano @ 2020-08-27 2:10 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jonathan Tan, git, Emily Shaffer
In-Reply-To: <20200827014723.GA750502@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Given the context above, two possibilities seem appealing:
>
> A. Like you're hinting, could dwim_ref get a variant that returns -1
> instead of die()ing on failure? That way, we could fulfill the
> intent described in b397ea48:
>
> When it cannot figure out the original ref, it shows an abbreviated
> SHA-1.
>
> B. Alternatively, in the @{u} case could we switch together the <old>
> and <new> pieces of the context from the
>
> checkout: moving from master to @{upstream}
>
> reflog line to make "master@{upstream}"? It's still possible for
> the upstream to have changed since then, but at least in the
> common case this would match the lookup that happened at checkout
> time.
Ah, blast from the past. Thanks for resurrecting.
If we are allowed to change what goes to reflog, can we do even
better than recording master@{upstream} at the time "checkout"
records the HEAD movement? "checkout: moving from next to master"
would be far better than "moving from next to next@{upstream}" or
"moving from next to @{upstream}".
Can we even change the phrasing altogether, like "checkout: moving
from next to detached e9b77c..."? That would produce even more
precise report.
^ permalink raw reply
* Re: [PATCH] wt-status: expand, not dwim, a "detached from" ref
From: Jonathan Nieder @ 2020-08-27 1:47 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Emily Shaffer
In-Reply-To: <20200513004058.34456-1-jonathantanmy@google.com>
Hi,
Jonathan Tan wrote:
> When a user checks out the upstream branch of HEAD and then runs "git
> branch", like this:
>
> git checkout @{u}
> git branch
>
> an error message is printed:
>
> fatal: HEAD does not point to a branch
Interesting. Even worse, it happens when I run "git status".
[...]
> This is because "git branch" attempts to DWIM "@{u}" when determining
> the "HEAD detached" message, but that doesn't work because HEAD no
> longer points to a branch.
>
> Therefore, when calculating the status of a worktree, only expand such a
> string, not DWIM it. Such strings would not match a ref, and "git
> branch" would report "HEAD detached at <hash>" instead.
[...]
> - if (dwim_ref(cb.buf.buf, cb.buf.len, &oid, &ref) == 1 &&
> + if (expand_ref(the_repository, cb.buf.buf, cb.buf.len, &oid, &ref) == 1 &&
> /* sha1 is a commit? match without further lookup */
Alas, as Junio mentions downthread, this patch produces a regression
in behavior: before,
$ git checkout --quiet master@{u}
$ git status | head -1
HEAD detached at origin/master
after,
$ git checkout --quiet master@{u}
$ git status | head -1
HEAD detached at 675a4aaf3b2
This ends up being a bit subtle.
The current branch can be in one of three states: I can be on a branch
(HEAD symref pointing to a ref refs/heads/branchname), detached (HEAD
psuedoref pointing to a commit), or on a branch yet to be born.
__git_ps1 in contrib/completion/git-prompt.sh, for example, is able to
show these three states.
Since b397ea4863a (status: show more info than "currently not on any
branch", 2013-03-13), "git status", on the other hand, gives richer
information. In the detached case, naming the commit that HEAD points
to doesn't really give a user enough information to orient themself,
so we look at the reflog (!) to find out what the user had intended to
check out. Reflog entries look like
checkout: moving from master to v1.0
and the "v1.0" can tell us that v1.0 is the tag that we detached HEAD
on.
This strikes me as always having been strange in a few ways:
On one hand, this is using the reflog. reflog entries are
historically meant to be human-readable. They are a raw string, not
structured data. They can be translated to a different human
language. Other tools interacting with git repositories are not
guaranteed to use the same string. Changes such as the introduction
of "git switch" could be expected to lead to writing "switch:" instead
of "checkout:" some day.
Beyond that, it involves guesswork. As b397ea4863a explains,
When it cannot figure out the original ref, it shows an
abbreviated SHA-1.
The reflog entry contains the parameter passed to "git checkout" in
the form the user provided --- it is not a full ref name or string
meant to be appended to refs/heads/ but it can be arbitrary syntax
supported by "git checkout". At the time that the checkout happened,
we know what *commit* that name resolved to, but we do not know what
ref it resolved to:
- refs in the search path can have been created or deleted since then
- with @{u} syntax, the named branch's upstream can have been
reconfigured
- and so on
If we want to know what ref HEAD was detached at, wouldn't we want
some reliable source of that information that records it at the time
it was known?
Another example is if I used "git checkout -" syntax. In that case,
the reflog records the commit id that I am checking out, instead of
recording "-". That's because (after "-" is replaced by "@{-1}") the
"magic branch" handler strbuf_branchname replaces @{-1} with with the
branch name being checked out. That branch name *also* comes from the
reflog, this time from the <old> side of the
checkout: moving from <old> to <new>
line, and <old> is populated as a simple commit id or branch name read
from HEAD. So this creates a bit of an inconsistency: if I run "git
status", "git checkout -", "git checkout -" again, and then "git
status" again, the first "git status" tells me what ref I had used to
detach HEAD but the second does not.
Okay, so much for background.
The motivating error producing
fatal: HEAD does not point to a branch
is a special case of the "we do not know what ref it resolved to"
problem described above: the string @{upstream} represents the
upstream of the branch that HEAD pointed to *at the time of the
checkout*, but since then HEAD has been detached.
[...]
> One alternative is to plumb down a flag from dwim_ref() to
> interpret_branch_mark() that indicates that (1) don't read HEAD when
> processing, and (2) when encountering a ref of the form "<optional
> ref>@{u}", don't die when the optional ref doesn't exist, is not a
> branch, or does not have an upstream.
Given the context above, two possibilities seem appealing:
A. Like you're hinting, could dwim_ref get a variant that returns -1
instead of die()ing on failure? That way, we could fulfill the
intent described in b397ea48:
When it cannot figure out the original ref, it shows an abbreviated
SHA-1.
B. Alternatively, in the @{u} case could we switch together the <old>
and <new> pieces of the context from the
checkout: moving from master to @{upstream}
reflog line to make "master@{upstream}"? It's still possible for
the upstream to have changed since then, but at least in the
common case this would match the lookup that happened at checkout
time.
Thoughts?
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH v2 0/2] avoid running "git-subcmd" in the dashed form
From: Junio C Hamano @ 2020-08-27 1:22 UTC (permalink / raw)
To: Derrick Stolee; +Cc: git
In-Reply-To: <07f26226-c1cd-494d-899e-d6452ad2751f@gmail.com>
Derrick Stolee <stolee@gmail.com> writes:
> Would an interesting in-between step include removing the dashed
> forms for builtins that didn't exist in Git 2.0?
I am not sure if it makes much sense to treat newer and older
built-in commands differently.
Imagine that an old-timer wrote a script by somebody who trusted the
"futz with PATH and you can use git-foo" promise before Git 2.0 and
then the script was inherited by relatively new users. Adhering to
the "when in doubt, mimic the surrounding code", which is usually a
good discipline to follow, these new users, who are now in charge of
maintaining the script, would add any new calls in "git-foo" form to
match the local convention in good faith. And the resulting code
would have been working just fine.
Before such a "in-between step" is thrown at them, that is, at which
point it stops working if they were unlucky that they used a
relatively new built-ins. Typically new end-users would not know
which ones are old built-ins and which ones are new, I suspect.
I do agree with Dscho that "we won't let you use builtin in dashed
form before you export an enviornment" I wrote was not a good way to
gauge the usage of on-disk builtins. We should move the on-disk
builtins to a different directory and have them point at the
location with their $PATH as the escape hatch, as Dscho suggested,
if we were to do this for real, I'd think.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 0/2] avoid running "git-subcmd" in the dashed form
From: Derrick Stolee @ 2020-08-27 0:57 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <20200826194650.4031087-1-gitster@pobox.com>
On 8/26/2020 3:46 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I think the cvsexportcommit and transport-helper changes are worth
>> salvaging even if we don't remove builtin binaries, so I'll split
>> them and whip them a bit more into a reasonable shape to be merged
>> to 'next'. The "break those who say 'git-cat-file'" can be left for
>> future.
>
> And here it is.
>
> These two patches are clean-ups that are worth doing whether we
> decide to remove on-disk binaries for the builtin commands.
LGTM.
> I droped the third one, that actually stops users from running
> built-in commands using the dashed form, at least for now. It can
> be resurrected later if we really wants to propose removal to the
> users, but I am not inclined to make such a proposal right now.
I think that's fine for now. A plan to fully deprecate the dashed
forms can come later.
Would an interesting in-between step include removing the dashed
forms for builtins that didn't exist in Git 2.0?
Thanks,
-Stolee
^ permalink raw reply
* [PATCH] builtin/index-pack.c: fix some sparse warnings
From: Ramsay Jones @ 2020-08-27 0:40 UTC (permalink / raw)
To: Jonathan Tan; +Cc: GIT Mailing-list
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Jonathan,
If you need to re-roll your 'jt/threaded-index-pack' branch, could you
please squash this into the relevant patch (commit 4a03e3d995 (index-pack:
make quantum of work smaller, 24-08-2020)).
Thanks!
ATB,
Ramsay Jones
builtin/index-pack.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3a20017102..8acd078aa0 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -65,7 +65,7 @@ struct base_data {
*
* Guarded by work_mutex.
*/
-LIST_HEAD(work_head);
+static LIST_HEAD(work_head);
/*
* Stack of struct base_data that have children, all of whom have been
@@ -75,7 +75,7 @@ LIST_HEAD(work_head);
*
* Guarded by work_mutex.
*/
-LIST_HEAD(done_head);
+static LIST_HEAD(done_head);
/*
* All threads share one delta base cache.
@@ -83,8 +83,8 @@ LIST_HEAD(done_head);
* base_cache_used is guarded by work_mutex, and base_cache_limit is read-only
* in a thread.
*/
-size_t base_cache_used;
-size_t base_cache_limit;
+static size_t base_cache_used;
+static size_t base_cache_limit;
struct thread_local {
pthread_t thread;
--
2.28.0
^ permalink raw reply related
* Re: post-checkout hook aborts rebase
From: Tom Rutherford @ 2020-08-27 0:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq3649szs8.fsf@gitster.c.googlers.com>
Thank you for the response Junio.
For what it's worth, my hook does not make changes to the repo. It's
running a command to check that the installed version of our
dependencies match the version specified in the commit being checked
out, and merely warns if the two don't match (then exits with a
nonzero return code).
For this reason it's been convenient that the hook runs during
rebases, but I find it surprising that the nonzero return code would
impact the rebase.
Tom
On Wed, Aug 26, 2020 at 5:22 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > ... If "git rebase" or whatever
> > command wanted to place files and the index into some state by using
> > "git checkout" command, and if the post-checkout hook mucked with
> > the state in such a way that contradicts with what the "git rebase"
> > command wanted them to be in, it is not surprising the hook's behavior
> > broke "git rebase"'s operation.
>
> Having said all that, I actually think that "rebase" shouldn't be
> invoking "git checkout" (and its equivalent) internally when
> switching to a specific version, in such a way that it would trigger
> any end-user specified hooks and allow them to muck with the working
> tree and the index state.
>
> I haven't checked the actual implementation of "git rebase" for
> quite some time to be sure, but we have lower-level plumbing
> commands that are not affected by the end-user hooks for exactly
> that kind of "build higher-level commands by synthesis of
> lower-level machinery", and it is very possible that what we are
> looking at is actually a bug that needs to be fixed. I dunno.
>
> Thanks.
^ permalink raw reply
* Re: post-checkout hook aborts rebase
From: Junio C Hamano @ 2020-08-27 0:22 UTC (permalink / raw)
To: Tom Rutherford; +Cc: git
In-Reply-To: <xmqq7dtlt080.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
> ... If "git rebase" or whatever
> command wanted to place files and the index into some state by using
> "git checkout" command, and if the post-checkout hook mucked with
> the state in such a way that contradicts with what the "git rebase"
> command wanted them to be in, it is not surprising the hook's behavior
> broke "git rebase"'s operation.
Having said all that, I actually think that "rebase" shouldn't be
invoking "git checkout" (and its equivalent) internally when
switching to a specific version, in such a way that it would trigger
any end-user specified hooks and allow them to muck with the working
tree and the index state.
I haven't checked the actual implementation of "git rebase" for
quite some time to be sure, but we have lower-level plumbing
commands that are not affected by the end-user hooks for exactly
that kind of "build higher-level commands by synthesis of
lower-level machinery", and it is very possible that what we are
looking at is actually a bug that needs to be fixed. I dunno.
Thanks.
^ permalink raw reply
* Re: post-checkout hook aborts rebase
From: Junio C Hamano @ 2020-08-27 0:13 UTC (permalink / raw)
To: Tom Rutherford; +Cc: git
In-Reply-To: <CAHr-Uu_KeAJZrd+GzymNP47iFi+dZkvVYsWQPtzT_FQrVnWTDg@mail.gmail.com>
Tom Rutherford <tmrutherford@gmail.com> writes:
> What did you expect to happen? (Expected behavior)
>
> The rebase succeeds.
>
> I expect this because the documentation for the post-checkout hook
> states, "This hook cannot affect the outcome of git switch or git
> checkout" https://www.git-scm.com/docs/githooks#_post_checkout
In general, pre-<anything> hook is run before <anything> subcommand
is executed, and by exiting with non-zero status, the hook can tell
the <anything> subcommand not to proceed. post-<anything> hook, on
the other hand, runs _after_ <anything> has done its thing already,
so by definition, it cannot say "Hey, <anything>, don't continue".
That is the primary meaning of "cannot affect" in that description.
Without looking at the contents of the actual hook is, nobody can
say anything definite, but it also means that "Your hook is NOT
ALLOWED TO do any extra modification to files and the index 'git
switch' or 'git checkout' made". If "git rebase" or whatever
command wanted to place files and the index into some state by using
"git checkout" command, and if the post-checkout hook mucked with
the state in such a way that contradicts with what the "git rebase"
command wanted them to be in, it is not surprising the hook's behavior
broke "git rebase"'s operation.
^ permalink raw reply
* Re: [PATCH v3 10/11] maintenance: add auto condition for commit-graph task
From: Junio C Hamano @ 2020-08-26 23:56 UTC (permalink / raw)
To: Jonathan Tan
Cc: gitgitgadget, git, sandals, steadmon, jrnieder, peff, congdanhqx,
phillip.wood123, emilyshaffer, sluongng, derrickstolee, dstolee
In-Reply-To: <20200826230252.1445841-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> Instead of writing a new commit-graph in every 'git maintenance run
>> --auto' process (when maintenance.commit-graph.enalbed is configured to
>> be true), only write when there are "enough" commits not in a
>> commit-graph file.
>>
>> This count is controlled by the maintenance.commit-graph.auto config
>> option.
>>
>> To compute the count, use a depth-first search starting at each ref, and
>> leaving markers using the PARENT1 flag. If this count reaches the limit,
>
> PARENT1 -> SEEN
>
> Other than that, all the 11 patches look good.
Thanks.
^ permalink raw reply
* post-checkout hook aborts rebase
From: Tom Rutherford @ 2020-08-26 23:10 UTC (permalink / raw)
To: git
Hi Git community,
My team encountered an issue today which I believe is a bug, and I'm
interested to know if those more familiar with the codebase and
documentation agree with me. Here's a bug report. Thanks for your
attention.
Tom
===
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
1. Have a post-checkout hook that returns a nonzero exit status.
2. `git pull --rebase` where there is at least one local commit to be
rebased onto an incoming commit. (I assume any rebase would exhibit
similar behavior, but this is how I encountered the issue).
What did you expect to happen? (Expected behavior)
The rebase succeeds.
I expect this because the documentation for the post-checkout hook
states, "This hook cannot affect the outcome of git switch or git
checkout" https://www.git-scm.com/docs/githooks#_post_checkout
What happened instead? (Actual behavior)
`error: could not detach HEAD` is displayed, and I end up in a
detached head state.
What's different between what you expected and what actually happened?
I do not expect the post-checkout hook to prevent rebase from
succeeding. The documentation suggests this should succeed. It seems
either the documentation is wrong, or git's use of the hook is in this
case.
Anything else you want to add:
The issue is elegantly explained in this stack overflow answer:
https://stackoverflow.com/a/25562688/1286571
It's not clear to me if this is a bug, or an omission from the
documentation. I reproduced it on 2.28, but I haven't tried building
from source.
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.28.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.4.0-42-generic #46~18.04.1-Ubuntu SMP Fri Jul 10
07:21:24 UTC 2020 x86_64
compiler info: gnuc: 7.5
libc info: glibc: 2.27
$SHELL (typically, interactive shell): /usr/bin/fish
[Enabled Hooks]
pre-commit
post-checkout
^ permalink raw reply
* Re: [PATCH v3 10/11] maintenance: add auto condition for commit-graph task
From: Jonathan Tan @ 2020-08-26 23:02 UTC (permalink / raw)
To: gitgitgadget
Cc: git, sandals, steadmon, jrnieder, peff, congdanhqx,
phillip.wood123, emilyshaffer, sluongng, jonathantanmy,
derrickstolee, dstolee
In-Reply-To: <4c3115fe3522bee47ba1f8f5e847e99ad7e56d40.1598380427.git.gitgitgadget@gmail.com>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Instead of writing a new commit-graph in every 'git maintenance run
> --auto' process (when maintenance.commit-graph.enalbed is configured to
> be true), only write when there are "enough" commits not in a
> commit-graph file.
>
> This count is controlled by the maintenance.commit-graph.auto config
> option.
>
> To compute the count, use a depth-first search starting at each ref, and
> leaving markers using the PARENT1 flag. If this count reaches the limit,
PARENT1 -> SEEN
Other than that, all the 11 patches look good.
^ permalink raw reply
* Re: [PATCH v3 07/11] maintenance: take a lock on the objects directory
From: Jonathan Tan @ 2020-08-26 23:02 UTC (permalink / raw)
To: gitgitgadget
Cc: git, sandals, steadmon, jrnieder, peff, congdanhqx,
phillip.wood123, emilyshaffer, sluongng, jonathantanmy,
derrickstolee, dstolee
In-Reply-To: <3d513acdd8885d90393cbfa847c38e802ffddac9.1598380427.git.gitgitgadget@gmail.com>
> If the lock file already exists, then fail with a warning. If '--auto'
> is specified, then instead no warning is shown and no tasks are attempted.
Maybe just delete these 2 lines - you're not failing, and you're
refraining from attempting all tasks regardless.
With or without this change, patches 1-7 look good.
^ permalink raw reply
* [PATCH] run_command: teach API users to use embedded 'args' more
From: Junio C Hamano @ 2020-08-26 22:25 UTC (permalink / raw)
To: git; +Cc: Jeff King
In-Reply-To: <xmqqzh6ht7fg.fsf_-_@gitster.c.googlers.com>
The child_process structure has an embedded strvec for formulating
the command line argument list these days, but code that predates
the wide use of it prepared a separate char *argv[] array and
manually set the child_process.argv pointer point at it.
Teach these old-style code to lose the separate argv[] array.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
convert.c | 5 +----
credential.c | 4 +---
submodule.c | 13 ++++---------
trailer.c | 4 +---
4 files changed, 7 insertions(+), 19 deletions(-)
diff --git a/convert.c b/convert.c
index 572449825c..8e6c292421 100644
--- a/convert.c
+++ b/convert.c
@@ -638,7 +638,6 @@ static int filter_buffer_or_fd(int in, int out, void *data)
struct child_process child_process = CHILD_PROCESS_INIT;
struct filter_params *params = (struct filter_params *)data;
int write_err, status;
- const char *argv[] = { NULL, NULL };
/* apply % substitution to cmd */
struct strbuf cmd = STRBUF_INIT;
@@ -656,9 +655,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)
strbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict);
strbuf_release(&path);
- argv[0] = cmd.buf;
-
- child_process.argv = argv;
+ strvec_push(&child_process.args, cmd.buf);
child_process.use_shell = 1;
child_process.in = -1;
child_process.out = out;
diff --git a/credential.c b/credential.c
index d8d226b97e..efc29dc5e1 100644
--- a/credential.c
+++ b/credential.c
@@ -274,11 +274,9 @@ static int run_credential_helper(struct credential *c,
int want_output)
{
struct child_process helper = CHILD_PROCESS_INIT;
- const char *argv[] = { NULL, NULL };
FILE *fp;
- argv[0] = cmd;
- helper.argv = argv;
+ strvec_push(&helper.args, cmd);
helper.use_shell = 1;
helper.in = -1;
if (want_output)
diff --git a/submodule.c b/submodule.c
index 01697848be..e6086faff7 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1727,14 +1727,6 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
int submodule_uses_gitfile(const char *path)
{
struct child_process cp = CHILD_PROCESS_INIT;
- const char *argv[] = {
- "submodule",
- "foreach",
- "--quiet",
- "--recursive",
- "test -f .git",
- NULL,
- };
struct strbuf buf = STRBUF_INIT;
const char *git_dir;
@@ -1747,7 +1739,10 @@ int submodule_uses_gitfile(const char *path)
strbuf_release(&buf);
/* Now test that all nested submodules use a gitfile too */
- cp.argv = argv;
+ strvec_pushl(&cp.args,
+ "submodule", "foreach", "--quiet", "--recursive",
+ "test -f .git", NULL);
+
prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
diff --git a/trailer.c b/trailer.c
index 0c414f2fed..68dabc2556 100644
--- a/trailer.c
+++ b/trailer.c
@@ -221,15 +221,13 @@ static char *apply_command(const char *command, const char *arg)
struct strbuf cmd = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
struct child_process cp = CHILD_PROCESS_INIT;
- const char *argv[] = {NULL, NULL};
char *result;
strbuf_addstr(&cmd, command);
if (arg)
strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
- argv[0] = cmd.buf;
- cp.argv = argv;
+ strvec_push(&cp.args, cmd.buf);
cp.env = local_repo_env;
cp.no_stdin = 1;
cp.use_shell = 1;
^ permalink raw reply related
* Re: [PATCH] pretty-options.txt: fix --no-abbrev-commit description
From: Junio C Hamano @ 2020-08-26 22:07 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <87pn7d9inu.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
>> Keeping the original sentence structure, e.g.
>>
>> ... and those options which imply abbreviating commit object names
>> such as ...
>>
>> would have been what I wrote, instead of "either explicit or implied
>> by", though.
>
> Sorry, but it'd then read:
>
> This negates `--abbrev-commit` and those options which imply
> abbreviating commit object names such as "--oneline".
>
> that again essentially reduces to:
>
> This negates "--oneline"
"--oneline" means a lot more than "do not use full object name", and
I think we are on the same page with our shared goal of not negating
everything "--oneline" means. We just want to say the option
negates only the "do not use full object name" aspect.
"and the effect of abbreviating commit objects implied by other
options, such as '--oneline'" may be a more verbose way to say the
same thing, I would think, but that would be overkill. I would have
expected that with common sense readers would think it would be
crazy for --no-abbrev to override everything --oneline means, but if
you found that the original risks such an interpretation, perhaps we
would need to be more verbose and explicit. I dunno.
^ permalink raw reply
* Re: [PATCH] pretty-options.txt: fix --no-abbrev-commit description
From: Sergey Organov @ 2020-08-26 21:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqv9h5uw7u.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> --no-abbrev-commit::
>> Show the full 40-byte hexadecimal commit object name. This negates
>> - `--abbrev-commit` and those options which imply it such as
>> - "--oneline". It also overrides the `log.abbrevCommit` variable.
>> + `--abbrev-commit`, either explicit or implied by other options such
>> + as "--oneline". It also overrides the `log.abbrevCommit` variable.
>
> I personally do not think it would be crazy to misread that one-line
> format itself would be defeated when no abbreviation of object names
> is requested, so from that point of view, the original text would be
> OK enough,
Well, I don't think it's OK to essentially say:
"--no-abbrev-commit" negates "--oneline"
when it doesn't. Yes, I even actually checked it doesn't, just in case.
> but as long as the updated text is easier to understand,
> that is fine ;-)
Admittedly, not being a native speaker, I'm not sure the form I used is
a good English, but at least it's factually correct.
Actually, I'd drop it altogether, to read like this:
--no-abbrev-commit::
Show the full 40-byte hexadecimal commit object name. This negates
`--abbrev-commit` as well as overrides the `log.abbrevCommit` variable.
as I don't see why such clarification is at all needed in the first
place.
I'll re-roll if you agree this variant is better.
>
> Keeping the original sentence structure, e.g.
>
> ... and those options which imply abbreviating commit object names
> such as ...
>
> would have been what I wrote, instead of "either explicit or implied
> by", though.
Sorry, but it'd then read:
This negates `--abbrev-commit` and those options which imply
abbreviating commit object names such as "--oneline".
that again essentially reduces to:
This negates "--oneline"
that is simply false statement, being exactly the problem of the
original that the patch tries to fix.
Thanks,
-- Sergey
^ permalink raw reply
* Re: [PATCH v8 3/5] rebase -i: support --committer-date-is-author-date
From: Junio C Hamano @ 2020-08-26 21:41 UTC (permalink / raw)
To: Phillip Wood
Cc: Johannes Schindelin, Elijah Newren, Rohit Ashiwal,
Đoàn Trần Công Danh, Alban Gruin,
Git Mailing List, Phillip Wood
In-Reply-To: <20200817174004.92455-4-phillip.wood123@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
> index 4f8a6e51c9..50a63d8ebe 100755
> --- a/t/t3436-rebase-more-options.sh
> +++ b/t/t3436-rebase-more-options.sh
> @@ -9,6 +9,9 @@ test_description='tests to ensure compatibility between am and interactive backe
>
> . "$TEST_DIRECTORY"/lib-rebase.sh
>
> +GIT_AUTHOR_DATE="1999-04-02T08:03:20+05:30"
> +export GIT_AUTHOR_DATE
> +
> # This is a special case in which both am and interactive backends
> # provide the same output. It was done intentionally because
> # both the backends fall short of optimal behaviour.
> @@ -21,11 +24,20 @@ test_expect_success 'setup' '
> test_write_lines "line 1" "new line 2" "line 3" >file &&
> git commit -am "update file" &&
> + ...
> + mkdir test-bin &&
> + write_script test-bin/git-merge-test <<-\EOF
> + exec git-merge-recursive "$@"
This should be
exec git merge-recursive "$@"
> + EOF
> '
^ permalink raw reply
* [PATCH v2 3/2] credential-cache: use child_process.args
From: Junio C Hamano @ 2020-08-26 21:37 UTC (permalink / raw)
To: git; +Cc: Jeff King
In-Reply-To: <20200826194650.4031087-3-gitster@pobox.com>
As child_process structure has an embedded strvec args for
formulating the command line, let's use it instead of using
an out-of-line argv[] whose length needs to be maintained
correctly.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/credential-cache.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
index d0fafdeb9e..195335a783 100644
--- a/builtin/credential-cache.c
+++ b/builtin/credential-cache.c
@@ -42,13 +42,13 @@ static int send_request(const char *socket, const struct strbuf *out)
static void spawn_daemon(const char *socket)
{
struct child_process daemon = CHILD_PROCESS_INIT;
- const char *argv[] = { NULL, NULL, NULL };
char buf[128];
int r;
- argv[0] = "git-credential-cache--daemon";
- argv[1] = socket;
- daemon.argv = argv;
+ strvec_pushl(&daemon.args,
+ "credential-cache--daemon", socket,
+ NULL);
+ daemon.git_cmd = 1;
daemon.no_stdin = 1;
daemon.out = -1;
^ permalink raw reply related
* Re: [PATCH v2] builtin/repack.c: invalidate MIDX only when necessary
From: Junio C Hamano @ 2020-08-26 20:54 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Taylor Blau, git, dstolee, peff, sluongng
In-Reply-To: <a9997a0b-9ba9-4614-2081-fc74e4c2171a@gmail.com>
Derrick Stolee <stolee@gmail.com> writes:
> On 8/25/2020 12:04 PM, Taylor Blau wrote:
>> In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'
>> learned to remove a multi-pack-index file if it added or removed a pack
>> from the object store.
>>
>> This mechanism is a little over-eager, since it is only necessary to
>> drop a MIDX if 'git repack' removes a pack that the MIDX references.
>> Adding a pack outside of the MIDX does not require invalidating the
>> MIDX, and likewise for removing a pack the MIDX does not know about.
>>
>> Teach 'git repack' to check for this by loading the MIDX, and checking
>> whether the to-be-removed pack is known to the MIDX. This requires a
>> slightly odd alternation to a test in t5319, which is explained with a
>> comment. A new test is added to show that the MIDX is left alone when
>> both packs known to it are marked as .keep, but two packs unknown to it
>> are removed and combined into one new pack.
>>
>> Helped-by: Derrick Stolee <dstolee@microsoft.com>
>> Signed-off-by: Taylor Blau <me@ttaylorr.com>
>> ---
>> Range-diff against v1:
>
> I know this thread went in a new direction about pack-redundant and
> dashed-git, but this version looks good to me. I wanted to make sure
> it wasn't lost in the shuffle.
Very much appreciated. I was wondering if I'll see a final reroll
or v2 already had all we wanted.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] builtin/repack.c: invalidate MIDX only when necessary
From: Derrick Stolee @ 2020-08-26 20:51 UTC (permalink / raw)
To: Taylor Blau, git; +Cc: dstolee, peff, sluongng
In-Reply-To: <87a3b7a5a2f091e2a23c163a7d86effbbbedfa3a.1598371475.git.me@ttaylorr.com>
On 8/25/2020 12:04 PM, Taylor Blau wrote:
> In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'
> learned to remove a multi-pack-index file if it added or removed a pack
> from the object store.
>
> This mechanism is a little over-eager, since it is only necessary to
> drop a MIDX if 'git repack' removes a pack that the MIDX references.
> Adding a pack outside of the MIDX does not require invalidating the
> MIDX, and likewise for removing a pack the MIDX does not know about.
>
> Teach 'git repack' to check for this by loading the MIDX, and checking
> whether the to-be-removed pack is known to the MIDX. This requires a
> slightly odd alternation to a test in t5319, which is explained with a
> comment. A new test is added to show that the MIDX is left alone when
> both packs known to it are marked as .keep, but two packs unknown to it
> are removed and combined into one new pack.
>
> Helped-by: Derrick Stolee <dstolee@microsoft.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> Range-diff against v1:
I know this thread went in a new direction about pack-redundant and
dashed-git, but this version looks good to me. I wanted to make sure
it wasn't lost in the shuffle.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing
From: Hariom verma @ 2020-08-26 15:18 UTC (permalink / raw)
To: Christian Couder
Cc: Eric Sunshine, Junio C Hamano, Hariom Verma via GitGitGadget,
Git List, Heba Waly
In-Reply-To: <CAP8UFD03Am94_84FvRPxEdt_AG74864eQ4TimggKtUYWjJYqCg@mail.gmail.com>
Hi,
On Wed, Aug 26, 2020 at 11:48 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> Hi,
>
> On Tue, Aug 25, 2020 at 1:32 AM Hariom verma <hariom18599@gmail.com> wrote:
>
> > On Mon, Aug 24, 2020 at 9:19 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> > > Aside from a couple minor style violations[1,2], I don't particularly
> > > oppose the helper function, though I have a quibble with the name
> > > check_format_field(), which I don't find helpful, and which (at least
> > > for me) increases the cognitive load. The increased cognitive load, I
> > > think, comes not only from the function name not spelling out what the
> > > function actually does, but also because the function is dual-purpose:
> > > it's both checking that the argument matches a particular token
> > > ("trailers", in this case) and extracting the sub-argument. Perhaps
> > > naming it match_and_extract_subarg() or something similar would help,
> > > though that's a mouthful.
> >
> > I will fix those violations.
> > Also, "match_and_extract_subarg()" looks good to me.
>
> I am not sure about the "subarg" part of the name. In the for-each-ref
> doc, names inside %(...) are called "field names", and parts after ":"
> are called "options". So it might be better to have "field_option"
> instead of "subarg" in the name.
>
> I think we could also get rid of the "match_and_" part of the
> suggestion, in the same way as skip_prefix() is not called
> match_and_skip_prefix(). Readers can just expect that if there is no
> match the function will return 0.
>
> So maybe "extract_field_option()".
Makes sense to me.
> > > But the observation about the function being dual-purpose (thus
> > > potentially confusing) brings up other questions. For instance, is it
> > > too special-purpose? If you foresee more callers in the future with
> > > multiple-token arguments such as `%(content:subject:sanitize)`, should
> > > the function provide more assistance by splitting out each of the
> > > sub-arguments rather than stopping at the first? Taking that even
> > > further, a generalized helper for "splitting" arguments like that
> > > might be useful at the top-level of contents_atom_parser() too, rather
> > > than only for specific arguments, such as "trailers". Of course, this
> > > may all be way too ambitious for this little bug fix series or even
> > > for whatever upcoming changes you're planning, thus not worth
> > > pursuing.
> >
> > Splitting sub-arguments is done at "<atomname>_atom_parser()".
> > If you mean pre-splitting every argument...
> > something like: ['contents', 'subject', 'sanitize'] for
> > `%(content:subject:sanitize)` in `contents_atom_parser()` ? I'm not
> > able to see how it can be useful.
>
> Yeah, it seems to me that such a splitting would require a complete
> rewrite of the current code, so I am not sure it's an interesting way
> forward for now. And anyway adding extract_field_option() goes in the
> right direction of abstracting the parsing and making the code
> simpler, more efficient and likely more correct.
>
> > Sorry, If I got your concerned wrong.
> >
> > > As for the helper's implementation, I might have written it like this:
> > >
> > > static int check_format_field(...)
> > > {
> > > const char *opt
> > > if (!strcmp(arg, field))
> > > *option = NULL;
> > > else if (skip_prefix(arg, field, opt) && *opt == ':')
> > > *option = opt + 1;
> > > else
> > > return 0;
> > > return 1;
> > > }
> > >
> > > which is more compact and closer to what I suggested earlier for
> > > avoiding the helper function in the first place. But, of course,
> > > programming is quite subjective, and you may find your implementation
> > > easier to reason about. Plus, your version has the benefit of being
> > > slightly more optimal since it avoids an extra string scan, although
> > > that probably is mostly immaterial considering that
> > > contents_atom_parser() itself contains a long chain of potentially
> > > sub-optimal strcmp() and skip_prefix() calls.
> >
> > "programming is quite subjective"
> > Yeah, I couldn't agree more.
> >
> > The change you suggested looks good too. But I'm little inclined to my
> > keeping my changes. I'm curious, what others have to say on this.
>
> I also prefer a slightly more optimal one even if it's a bit less compact.
+1
Thanks,
Hariom
^ permalink raw reply
* Re: [PATCH] revision: set rev_input_given in handle_revision_arg()
From: Junio C Hamano @ 2020-08-26 20:20 UTC (permalink / raw)
To: Jeff King; +Cc: Bryan Turner, Git Users
In-Reply-To: <20200826201305.GA1595824@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Subject: [PATCH] revision: set rev_input_given in handle_revision_arg()
>
> Commit 7ba826290a (revision: add rev_input_given flag, 2017-08-02) added
> a flag to rev_info to tell whether we got any revision arguments. As
> explained there, this is necessary because some revision arguments may
> not produce any pending traversal objects, but should still inhibit
> default behaviors (e.g., a glob that matches nothing).
Ah, that explains the symptom under discussion quite well; the topic
that introduced the commit was to fix "git log --tag=no-such-tag"
that used to default to HEAD. We should be able to reuse the same
mechanism to prevent --stdin codepath from falling back to HEAD, of
course.
> However, it only set the flag in the globbing code, but not for
> revisions we get on the command-line or via stdin. This leads to two
> problems:
>
> - the command-line code keeps its own separate got_rev_arg flag; this
> isn't wrong, but it's confusing and an extra maintenance burden
>
> - even specifically-named rev arguments might end up not adding any
> pending objects: if --ignore-missing is set, then specifying a
> missing object is a noop rather than an error.
Yup, nicely described.
> diff --git a/revision.c b/revision.c
> index 96630e3186..08c2ad23af 100644
> --- a/revision.c
> +++ b/revision.c
The patch of course looks straightforward and good. Thanks.
^ permalink raw reply
* [PATCH] revision: set rev_input_given in handle_revision_arg()
From: Jeff King @ 2020-08-26 20:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Bryan Turner, Git Users
In-Reply-To: <20200825195511.GD1419759@coredump.intra.peff.net>
On Tue, Aug 25, 2020 at 03:55:11PM -0400, Jeff King wrote:
> > You beat me to it while I was wondering what to do between the local
> > got_rev_arg variable and the revs->rev_input_given field.
>
> That makes me wonder why we need got_rev_arg at all if we have
> revs->rev_input_given. But I suspect an answer can be found by digging
> into git-blame. I probably won't do that immediately, so if you want to,
> you can do so without worrying that we're duplicating work. :)
This is all my fault, naturally. :) I added rev_input_given a while ago
but didn't go far enough. Here's what I think we should do:
-- >8 --
Subject: [PATCH] revision: set rev_input_given in handle_revision_arg()
Commit 7ba826290a (revision: add rev_input_given flag, 2017-08-02) added
a flag to rev_info to tell whether we got any revision arguments. As
explained there, this is necessary because some revision arguments may
not produce any pending traversal objects, but should still inhibit
default behaviors (e.g., a glob that matches nothing).
However, it only set the flag in the globbing code, but not for
revisions we get on the command-line or via stdin. This leads to two
problems:
- the command-line code keeps its own separate got_rev_arg flag; this
isn't wrong, but it's confusing and an extra maintenance burden
- even specifically-named rev arguments might end up not adding any
pending objects: if --ignore-missing is set, then specifying a
missing object is a noop rather than an error.
And that leads to some user-visible bugs:
- when deciding whether a default rev like "HEAD" should kick in, we
check both got_rev_arg and rev_input_given. That means that
"--ignore-missing $ZERO_OID" works on the command-line (where we set
got_rev_arg) but not on --stdin (where we don't)
- when rev-list decides whether it should complain that it wasn't
given a starting point, it relies on rev_input_given. So it can't
even get the command-line "--ignore-missing $ZERO_OID" right
Let's consistently set the flag if we got any revision argument. That
lets us clean up the redundant got_rev_arg, and fixes both of those bugs
(but note there are three new tests: we'll confirm the already working
git-log command-line case).
A few implementation notes:
- conceptually we want to set the flag whenever handle_revision_arg()
finds an actual revision arg ("handles" it, you might say). But it
covers a ton of cases with early returns. Rather than annotating
each one, we just wrap it and use its success exit-code to set the
flag in one spot.
- the new rev-list test is in t6018, which is titled to cover globs.
This isn't exactly a glob, but it made sense to stick it with the
other tests that handle the "even though we got a rev, we have no
pending objects" case, which are globs.
- the tests check for the oid of a missing object, which it's pretty
clear --ignore-missing should ignore. You can see the same behavior
with "--ignore-missing a-ref-that-does-not-exist", because
--ignore-missing treats them both the same. That's perhaps less
clearly correct, and we may want to change that in the future. But
the way the code and tests here are written, we'd continue to do the
right thing even if it does.
Reported-by: Bryan Turner <bturner@atlassian.com>
Signed-off-by: Jeff King <peff@peff.net>
---
revision.c | 16 +++++++++++-----
t/t4202-log.sh | 10 ++++++++++
t/t6018-rev-list-glob.sh | 5 +++++
3 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/revision.c b/revision.c
index 96630e3186..08c2ad23af 100644
--- a/revision.c
+++ b/revision.c
@@ -2017,7 +2017,7 @@ static int handle_dotdot(const char *arg,
return ret;
}
-int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)
+static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)
{
struct object_context oc;
char *mark;
@@ -2092,6 +2092,14 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
return 0;
}
+int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
+{
+ int ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);
+ if (!ret)
+ revs->rev_input_given = 1;
+ return ret;
+}
+
static void read_pathspec_from_stdin(struct strbuf *sb,
struct strvec *prune)
{
@@ -2703,7 +2711,7 @@ static void NORETURN diagnose_missing_default(const char *def)
*/
int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *opt)
{
- int i, flags, left, seen_dashdash, got_rev_arg = 0, revarg_opt;
+ int i, flags, left, seen_dashdash, revarg_opt;
struct strvec prune_data = STRVEC_INIT;
const char *submodule = NULL;
int seen_end_of_options = 0;
@@ -2792,8 +2800,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
strvec_pushv(&prune_data, argv + i);
break;
}
- else
- got_rev_arg = 1;
}
if (prune_data.nr) {
@@ -2822,7 +2828,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
opt->tweak(revs, opt);
if (revs->show_merge)
prepare_show_merge(revs);
- if (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {
+ if (revs->def && !revs->pending.nr && !revs->rev_input_given) {
struct object_id oid;
struct object *object;
struct object_context oc;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index a0930599aa..56d34ed465 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1850,6 +1850,16 @@ test_expect_success 'log does not default to HEAD when rev input is given' '
test_must_be_empty actual
'
+test_expect_success 'do not default to HEAD with ignored object on cmdline' '
+ git log --ignore-missing $ZERO_OID >actual &&
+ test_must_be_empty actual
+'
+
+test_expect_success 'do not default to HEAD with ignored object on stdin' '
+ echo $ZERO_OID | git log --ignore-missing --stdin >actual &&
+ test_must_be_empty actual
+'
+
test_expect_success 'set up --source tests' '
git checkout --orphan source-a &&
test_commit one &&
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index bb5aeac07f..b31ff7eeec 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -345,6 +345,11 @@ test_expect_success 'rev-list should succeed with empty output with empty glob'
test_must_be_empty actual
'
+test_expect_success 'rev-list should succeed with empty output when ignoring missing' '
+ git rev-list --ignore-missing $ZERO_OID >actual &&
+ test_must_be_empty actual
+'
+
test_expect_success 'shortlog accepts --glob/--tags/--remotes' '
compare shortlog "subspace/one subspace/two" --branches=subspace &&
--
2.28.0.749.gf1242ce4bd
^ 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