Re: [PATCH 4/4] submodule deinit: unset core.worktree
Stefan Beller writes:
> This re-introduces 984cd77ddb (submodule deinit: unset core.worktree,
> 2018-06-18), which was reverted as part of f178c13fda (Revert "Merge
> branch 'sb/submodule-core-worktree'", 2018-09-07)
>
> The whole series was reverted as the offending commit e98317508c
> (submodule: ensure core.worktree is set after update, 2018-06-18)
> was relied on by other commits such as 984cd77ddb.
>
> Keep the offending commit reverted, but its functionality came back via
> 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17), such
> that we can reintroduce 984cd77ddb now.
None of the above three explains the most important thing directly,
so readers fail to grasp what the main theme of the three-patch
series is, without looking at the commits that were reverted
already.
Is the theme of the overall series to make sure core.worktree is set
to point at the working tree when submodule's working tree is
instantiated, and unset it when it is not?
2/4 was also explained (in the original) that it wants to unset and
did so when "move_head" gets called. This one does the unset when a
submodule is deinited. Are these the only two cases a submodule
loses its working tree? If so, the log message for this step should
declare victory by ending with something like
... as we covered the only other case in which a submodule
loses its working tree in the earlier step (i.e. switching
branches of top-level project to move to a commit that did
not have the submodule), this makes the code always maintain
core.worktree correctly unset when there is no working tree
for a submodule.
Thanks. I think I agree with what the series wants to do (if I read
what it wants to do correctly, that is).
> Signed-off-by: Stefan Beller
> ---
> builtin/submodule--helper.c | 2 ++
> t/lib-submodule-update.sh | 2 +-
> t/t7400-submodule-basic.sh | 5 +
> 3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 31ac30cf2f..672b74db89 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1131,6 +1131,8 @@ static void deinit_submodule(const char *path, const
> char *prefix,
> if (!(flags & OPT_QUIET))
> printf(format, displaypath);
>
> + submodule_unset_core_worktree(sub);
> +
> strbuf_release(_rm);
> }
>
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index 51d449..5b56b23166 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -235,7 +235,7 @@ reset_work_tree_to_interested () {
> then
> mkdir -p submodule_update/.git/modules/sub1/modules &&
> cp -r submodule_update_repo/.git/modules/sub1/modules/sub2
> submodule_update/.git/modules/sub1/modules/sub2
> - GIT_WORK_TREE=. git -C
> submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree
> + # core.worktree is unset for sub2 as it is not checked out
> fi &&
> # indicate we are interested in the submodule:
> git -C submodule_update config submodule.sub1.url "bogus" &&
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 76a7cb0af7..aba2d4d6ee 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -984,6 +984,11 @@ test_expect_success 'submodule deinit should remove the
> whole submodule section
> rmdir init
> '
>
> +test_expect_success 'submodule deinit should unset core.worktree' '
> + test_path_is_file .git/modules/example/config &&
> + test_must_fail git config -f .git/modules/example/config core.worktree
> +'
> +
> test_expect_success 'submodule deinit from subdirectory' '
> git submodule update --init &&
> git config submodule.example.foo bar &&
Re: [PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree
Stefan Beller writes:
> Shortly after f178c13fda (Revert "Merge branch
> 'sb/submodule-core-worktree'", 2018-09-07), we had another series
> that implemented partially the same, ensuring that core.worktree was
> set in a checked out submodule, namely 74d4731da1 (submodule--helper:
> replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)
>
> As the series 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c',
> 2018-09-17) has different goals than the reverted series 7e25437d35
> (Merge branch 'sb/submodule-core-worktree', 2018-07-18), I'd wanted to
> replay the series on top of it to reach the goal of having `core.worktree`
> correctly set when the submodules worktree is present, and unset when the
> worktree is not present.
>
> The replay resulted in a strange merge conflict highlighting that
> the BUG message was not changed in 74d4731da1 (submodule--helper:
> replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13).
>
> Fix the error message.
>
> Signed-off-by: Stefan Beller
> ---
Unlike the step 2/4 I commented on, this does explain what this
wants to do and why, at least when looked from sideways. Is the
above saying the same as the following two-liner?
An ealier mistake while rebasing to produce 74d4731da1
failed to update this BUG message. Fix this.
> builtin/submodule--helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d38113a31a..31ac30cf2f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2045,7 +2045,7 @@ static int ensure_core_worktree(int argc, const char
> **argv, const char *prefix)
> struct repository subrepo;
>
> if (argc != 2)
> - BUG("submodule--helper connect-gitdir-workingtree
> ");
> + BUG("submodule--helper ensure-core-worktree ");
>
> path = argv[1];
Re: [PATCH 2/4] submodule: unset core.worktree if no working tree is present
Stefan Beller writes:
> This reintroduces 4fa4f90ccd (submodule: unset core.worktree if no working
> tree is present, 2018-06-12), which was reverted as part of f178c13fda
> (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07).
>
> 4fa4f90ccd was reverted as its followup commit was faulty, but without
> the accompanying change of the followup, we'd have an incomplete workflow
> of setting `core.worktree` again, when it is needed such as checking out
> a revision that contains a submodule.
>
> So re-introduce that commit as-is, focusing on fixing up the followup
I was hoping to hear (given what 0/4 claimed) a clearer explanation
of what this change wants to achieve, but that is lacking.
No need to grumble about an earlier work was that turned out to be
inappropriate for the codebase back then. Repeatedly saying "this
is needed" without giving further explaining why it is so, or
anything like that, would help readers.
Just pretend that the ealier commits and their reversion never
happened, and further pretend that we are doing the best thing that
should happen to our codebase. How would we explain this change,
what the problem it tries to solve and what the solution looks like
in the larger picture?
When removing a working tree of a submodule (e.g. we may be
switching back to an earlier commit in the superproject that
did not have the submodule in question yet), we failed to
unset core.worktree of the submodule's repository. That
caused this and that issues, exhibited by a few new tests
this commit adds.
Make sure that core.worktree gets unset so that a leftover
setting won't cause these issues.
or something like that? I am just guessing by looking at the old
commit's text, as the above two paragraphs and one sentence does not
say much.
> diff --git a/submodule.c b/submodule.c
> index 6415cc5580..d393e947e6 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1561,6 +1561,18 @@ int bad_to_remove_submodule(const char *path, unsigned
> flags)
> return ret;
> }
>
> +void submodule_unset_core_worktree(const struct submodule *sub)
> +{
> + char *config_path = xstrfmt("%s/modules/%s/config",
> + get_git_common_dir(), sub->name);
> +
> + if (git_config_set_in_file_gently(config_path, "core.worktree", NULL))
> + warning(_("Could not unset core.worktree setting in submodule
> '%s'"),
> + sub->path);
> +
> + free(config_path);
> +}
> +
> static const char *get_super_prefix_or_empty(void)
> {
> const char *s = get_super_prefix();
> @@ -1726,6 +1738,8 @@ int submodule_move_head(const char *path,
>
> if (is_empty_dir(path))
> rmdir_or_warn(path);
> +
> + submodule_unset_core_worktree(sub);
> }
> }
> out:
> diff --git a/submodule.h b/submodule.h
> index a680214c01..9e18e9b807 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -131,6 +131,8 @@ int submodule_move_head(const char *path,
> const char *new_head,
> unsigned flags);
>
> +void submodule_unset_core_worktree(const struct submodule *sub);
> +
> /*
> * Prepare the "env_array" parameter of a "struct child_process" for
> executing
> * a submodule by clearing any repo-specific environment variables, but
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index 016391723c..51d449 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -709,7 +709,8 @@ test_submodule_recursing_with_args_common() {
> git branch -t remove_sub1 origin/remove_sub1 &&
> $command remove_sub1 &&
> test_superproject_content origin/remove_sub1 &&
> - ! test -e sub1
> + ! test -e sub1 &&
> + test_must_fail git config -f .git/modules/sub1/config
> core.worktree
> )
> '
> # ... absorbing a .git directory along the way.
Re: [RFC PATCH 2/3] Documentation/git-rev-list: s///
Matthew DeVore writes: > $ git -C pc1 fetch-pack --stdin "file://$(pwd)/srv.bare" > Where observed.oids contains all the blobs that were missing. It tells > the remote that it already has the "refs/heads/master" commit, which > means it is excluded. Before, this worked fine, since it didn't mean > the blobs were excluded, only the commit itself. So 'master' has some blob in its tree, which is missing from the repository, in this test? Which means that such a repository is "corrupt" and does not pass connectivity check by fsck. I am of two minds. If we claim that we have everything that is needed to complete the commit sitting at the tip of 'master', I think it is correct for the other side not to send a blob that is in 'master' (or its ancestors), so your "fix" may (at least technically) be more correct than the status quo. On the other hand, if possession of commit 'master' does not defeat an explicit request for a blob in it, that would actually be a good thing---it would be a very straight-forward way to recover from such form of repository corruption. Fetching isolated objects without walking is also something that would help backfill a lazily created clone, and I even vaguely recall an effort to allow objects explicitly requested to be always fetched regardless of the connectivity, if I am not mistaken (JTan?) Anyway, thanks for a thoughtful response.
Re: [PATCH] mailmap: update brandon williams's email address
Stefan Beller writes: > On Fri, Dec 7, 2018 at 1:40 PM Jonathan Nieder wrote: >> >> Brandon Williams wrote: >> >> > Signed-off-by: Brandon Williams >> > --- >> > .mailmap | 1 + >> > 1 file changed, 1 insertion(+) >> >> I can confirm that this is indeed the same person. > > What would be more of interest is why we'd be interested in this patch > as there is no commit/patch sent by Brandon with this email in gits history. Once I "git am" the message that began this thread, there will be a commit under this new ident, so that would be somewhat a moot point. If this were "Jonathan asked Brandon if we want to record an address we can reach him in our .mailmap file and sent a patch to add one", then the story is different, and I tend to agree with you that such a patch is more or less pointless. That's not the purpose of the mailmap file. Not until git-send-email learns to use that file to rewrite To/cc/etc to the "canonical" addresses, anyway ;-) I am not sure if there are people whose "canonical" address to be used as the author is not necessarily the best address they want to get their e-mails at, though. If we can be reasonably sure that the set of such people is empty, then people can take the above mention about send-email as a hint about a low-hanging fruit ;-) Thanks.
Re: RFE: git-patch-id should handle patches without leading "diff"
Jonathan Nieder writes: >> So it seems most sensible to me if this is going to be supported that we >> go a bit beyond the call of duty and fake up the start of it, namely: >> >> --- a/arch/x86/kernel/process.c >> +++ b/arch/x86/kernel/process.c >> >> To be: >> >> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c >> --- a/arch/x86/kernel/process.c >> +++ b/arch/x86/kernel/process.c > > Right. We may want to handle diff.mnemonicPrefix as well. I definitely think under the --stable option, we should pretend as if the canonical a/ vs b/ prefixes were given with the "diff --git" header, just like we try to reverse the effect of diff-orderfile, etc. I am unsure what the right behaviour under --unstable is, though.
Re: [PATCH 0/4]
Stefan Beller writes: > A couple days before the 2.19 release we had a bug report about > broken submodules[1] and reverted[2] the commits leading up to them. > > The behavior of said bug fixed itself by taking a different approach[3], > specifically by a weaker enforcement of having `core.worktree` set in a > submodule [4]. > > The revert [2] was overly broad as we neared the release, such that we wanted > to rather keep the known buggy behavior of always having `core.worktree` set, > rather than figuring out how to fix the new bug of having 'git submodule > update' > not working in old style repository setups. > > This series re-introduces those reverted patches, with no changes in code, > but with drastically changed commit messages, as those focus on why it is safe > to re-introduce them instead of explaining the desire for the change. The above was a bit too cryptic for me to grok, so let me try rephrasing to see if I got them all correctly. - three-patch series leading to 984cd77ddb were meant to fix some bug, but the series itself was buggy and caused problems; we got rid of them - the problem 984cd77ddb wanted to fix was fixed differently without reintroducing the problem three-patch series introduced. That fix is already with us since 4d6d6ef1fc. - now these three changes that were problematic in the past is resent without any update (other than that it has one preparatory patch to add tests). Is that what is going on? Obviously I am not getting "the other" benefit we wanted to gain out of these three patches (because the above description fails to explain what that is), other than to fix the issue that was fixed by 4d6d6ef1fc. Sorry for being puzzled... > [1] https://public-inbox.org/git/2659750.rG6xLiZASK@twilight > [2] f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'", > 2018-09-07) > [3] 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17) > [4] 74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by > ensure-core-worktree, 2018-08-13) > > Stefan Beller (4): > submodule update: add regression test with old style setups > submodule: unset core.worktree if no working tree is present > submodule--helper: fix BUG message in ensure_core_worktree > submodule deinit: unset core.worktree > > builtin/submodule--helper.c| 4 +++- > submodule.c| 14 ++ > submodule.h| 2 ++ > t/lib-submodule-update.sh | 5 +++-- > t/t7400-submodule-basic.sh | 5 + > t/t7412-submodule-absorbgitdirs.sh | 7 ++- > 6 files changed, 33 insertions(+), 4 deletions(-)
Re: [PATCH] commit: abort before commit-msg if empty message
Jonathan Tan writes: > When a user runs "git commit" without specifying a message, an editor > appears with advice: > > Please enter the commit message for your changes. Lines starting > with '#' will be ignored, and an empty message aborts the commit. > > However, if the user supplies an empty message and has a commit-msg hook > which updates the message to be non-empty, the commit proceeds to occur, > despite what the advice states. When "--no-edit" is given, and when commit-msg fills that blank, the command should go ahead and record the commit, I think. An automation where commit-msg is used to produce whatever appropriate message for the automation is entirely a reasonable thing to arrange. Of course, you can move the logic to produce an appropriate message for the automation from commit-msg to the script that drives the "git commit" and use the output of that logic as the value for the "-m" option to achieve the same, so in that sense, there is an escape hatch even if you suddenly start to forbid such a working set-up, but it nevertheless is unnecessary busywork for those with such a set-up to adjust to this change. I actually think in this partcular case, the commit-msg hook that adds Change-ID to an empty message is BUGGY. If the hook looked at the message contents and refrains from making an otherwise empty message to non-empty, there is no need for any change here. In any case, you'll have plenty of time to make your case after the rc freeze. I am not so sympathetic to a patch that makes us bend backwards to support such a buggy hook to e honest.
Re: [WIP RFC 4/5] upload-pack: refactor writing of "packfile" line
Jonathan Tan writes:
>> Jonathan Tan writes:
>>
>> > @@ -126,6 +129,12 @@ static int read_pack_objects_stdout(int outfd, struct
>> > output_state *os)
>> >}
>> >os->used += readsz;
>> >
>> > + if (!os->packfile_started) {
>> > + os->packfile_started = 1;
>> > + if (use_protocol_v2)
>> > + packet_write_fmt(1, "packfile\n");
>>
>> If we fix this function so that the only byte in the buffer is held
>> back without emitted when os->used == 1 as I alluded to, this may
>> have to be done a bit later, as with such a change, it is no longer
>> guaranteed that send_client_data() will be called after this point.
>
> I'm not sure what you mean about there being no guarantee that
> send_client_data() is not called - in create_pack_file(), there is an
> "if (output_state.used > 0)" line (previously "if (0 <= buffered)") that
> outputs anything remaining.
I was referring to this part of the review on the previous step,
which you may not yet have read.
OK, this corresponds to the "*cp++ = buffered" in the original just
before xread().
> + os->used = 1;
> + } else {
> + send_client_data(1, os->buffer, os->used);
> + os->used = 0;
I am not sure if the code is correct when os->used happens to be 1
(shouldn't we hold the byte, skip the call to send_client_data(),
and go back to poll() to expect more data?), but this is a faithful
code movement and rewrite of the original.
The point of this logic is to make sure we always hold back some
bytes and do not feed *all* the bytes to the other side by calling
"send-client-data" until we made sure the upstream of what we are
relaying (pack-objects?) successfully exited, but it looks to me
that the "else" clause above ends up flushing everything when
os->used is 1, which goes against the whole purpose of the code.
And the "fix" I was alluding to was to update that "else" clause to
make it a no-op that keeps os->used non-zero, which would not call
send-client-data.
When that fix happens, the part that early in the function this
patch added "now we know we will call send-client-data, so let's say
'here comes packdata' unless we have already said that" is making
the decision too early. Depending on the value of os->used when we
enter the code and the number of bytes xread() reads from the
upstream, we might not call send-client-data yet (namely, when we
have no buffered data and we happened to get only one byte).
> ... it might be
> better if the server can send sideband throughout the whole response -
> perhaps that should be investigated first.
Yup. It just looked quite crazy, and it is even more crazy to
buffer keepalives ;-)
Re: Any way to make git-log to enumerate commits?
Konstantin Khomoutov writes:
>> I do not see why the "name each rev relative to HEAD" formatting
>> option cannot produce HEAD^2~2 etc.
>> ...
> My reading was that the OP explicitly wanted to just glance at a single
> integer number and use it right away in a subsequent rebase command.
>
> I mean, use one's own short memory instead of copying and pasting.
As everybody pointed out, a single integer number will fundamentally
unworkable with distributed nature of Git that inherently makes the
history with forks and merges. Besides, there is no way to feed
"git log" with "a single integer number", without which "making
git-log to enumerate" would not be all that useful ("git show
HEAD~4" works but "git show 4" does not).
All of these name-rev based suggestions were about using anchoring
point with memorable name plus a short path to reach from there,
which I think is the closest thing to "a single integer number" and
still is usable for the exact purpose. "HEAD~48^2" on an
integration branch would be "the tip of the side branch that was
merged some 48 commits ago", for example.
Re: [RFC PATCH 3/3] test-lib: add the '--stress' option to run a test repeatedly under load
Jeff King writes: > Each "wait" will try to collect all processes, but may be interrupted by > a signal. So the correct number is actually "1 plus the number of times > the user hits ^C". Yeah and that is not bounded. It is OK not to catch multiple ^C that races with what we do, so having ane extra wait in the clean-up procedure after receiving a signal like you suggested would be both good enough and the cleanest solution, I think. Thanks.
Re: [WIP RFC 4/5] upload-pack: refactor writing of "packfile" line
Jonathan Tan writes:
> @@ -126,6 +129,12 @@ static int read_pack_objects_stdout(int outfd, struct
> output_state *os)
> }
> os->used += readsz;
>
> + if (!os->packfile_started) {
> + os->packfile_started = 1;
> + if (use_protocol_v2)
> + packet_write_fmt(1, "packfile\n");
If we fix this function so that the only byte in the buffer is held
back without emitted when os->used == 1 as I alluded to, this may
have to be done a bit later, as with such a change, it is no longer
guaranteed that send_client_data() will be called after this point.
> + }
> +
> if (os->used > 1) {
> send_client_data(1, os->buffer, os->used - 1);
> os->buffer[0] = os->buffer[os->used - 1];
> +static void flush_progress_buf(struct strbuf *progress_buf)
> +{
> + if (!progress_buf->len)
> + return;
> + send_client_data(2, progress_buf->buf, progress_buf->len);
> + strbuf_reset(progress_buf);
> +}
Interesting.
> static void create_pack_file(const struct object_array *have_obj,
> - const struct object_array *want_obj)
> + const struct object_array *want_obj,
> + int use_protocol_v2)
> {
> struct child_process pack_objects = CHILD_PROCESS_INIT;
> struct output_state output_state = {0};
> @@ -260,9 +278,13 @@ static void create_pack_file(const struct object_array
> *have_obj,
>*/
> sz = xread(pack_objects.err, progress,
> sizeof(progress));
> - if (0 < sz)
> - send_client_data(2, progress, sz);
> - else if (sz == 0) {
> + if (0 < sz) {
> + if (output_state.packfile_started)
> + send_client_data(2, progress, sz);
> + else
> + strbuf_add(_state.progress_buf,
> +progress, sz);
Isn't progress output that goes to the channel #2 pretty much
independent from the payload stream that carries the pkt-line
command like "packfile" plus the raw pack stream? It somehow
feels like an oxymoron to _buffer_ progress indicator, as it
defeats the whole point of progress report to buffer it.
Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph
Josh Steadmon writes:
> diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c
> new file mode 100644
> index 00..420851d0d2
> --- /dev/null
> +++ b/fuzz-commit-graph.c
> @@ -0,0 +1,18 @@
> +#include "object-store.h"
> +#include "commit-graph.h"
> +
> +struct commit_graph *parse_commit_graph(void *graph_map, int fd,
> + size_t graph_size);
> +
> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
> +
> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
> +{
> + struct commit_graph *g;
> +
> + g = parse_commit_graph((void *) data, -1, size);
> + if (g)
> + free(g);
As it is perfectly OK to free(NULL), please lose "if (g)" and a
level of indentation; otherwise, "make coccicheck" would complain.
Thanks.
> + return 0;
> +}
Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph
Junio C Hamano writes: >> +if (graph_size < GRAPH_MIN_SIZE) >> +return NULL; >> + > > The load_commit_graph_one() grabbed graph_map out of xmmap() so it > is guaranteed to be non-NULL, but we need to check graph_map != NULL > when we're calling this directly from the fuzz tests, exactly in the > same spirit that we check graph_size above. Besides, these are to > make sure future callers won't misuse the API. Insert "Please check graph_map != NULL here, too." before the above paragraph. >> data = (const unsigned char *)graph_map; > > And the reset of the function is the same as the original modulo > jumping to the cleanup_fail label has been replaced with returning > NULL. > > Looks good. Of course, s/reset/rest/ is what I meant.
Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph
Josh Steadmon writes:
> @@ -108,27 +106,61 @@ struct commit_graph *load_commit_graph_one(const char
> *graph_file)
> die(_("graph file %s is too small"), graph_file);
> }
> graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
> + ret = parse_commit_graph(graph_map, fd, graph_size);
OK, assuming that the new helper returns NULL from all places in the
original that would have jumped to the cleanup-fail label, this would
do the same thing (it may not be the right thing to exit, but that
is OK for the purpose of this change).
> + if (ret == NULL) {
> + munmap(graph_map, graph_size);
> + close(fd);
> + exit(1);
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * This function is intended to be used only from load_commit_graph_one() or
> in
> + * fuzz tests.
> + */
Hmph, is that necessary to say?
"Right now, it has only these two callers" is sometimes handy for
those without good static analysis tools, like "git grep" ;-), but I
do not think of a reason why a new caller we'll add in the future,
who happens to have the data of the graph file in-core, should not
be allowed to call this function.
> +struct commit_graph *parse_commit_graph(void *graph_map, int fd,
> + size_t graph_size)
> +{
> + const unsigned char *data, *chunk_lookup;
> + uint32_t i;
> + struct commit_graph *graph;
> + uint64_t last_chunk_offset;
> + uint32_t last_chunk_id;
> + uint32_t graph_signature;
> + unsigned char graph_version, hash_version;
> +
> + /*
> + * This should already be checked in load_commit_graph_one, but we still
> + * need a check here for when we're calling parse_commit_graph directly
> + * from fuzz tests. We can omit the error message in that case.
> + */
In the same spirit, I am dubious of the longer-term value of this
comment. As an explanation of why this conversion is correct in the
proposed log message for this change, it perfectly makes sense,
though.
> + if (graph_size < GRAPH_MIN_SIZE)
> + return NULL;
> +
The load_commit_graph_one() grabbed graph_map out of xmmap() so it
is guaranteed to be non-NULL, but we need to check graph_map != NULL
when we're calling this directly from the fuzz tests, exactly in the
same spirit that we check graph_size above. Besides, these are to
make sure future callers won't misuse the API.
> data = (const unsigned char *)graph_map;
And the reset of the function is the same as the original modulo
jumping to the cleanup_fail label has been replaced with returning
NULL.
Looks good.
> ...
> -
> -cleanup_fail:
> - munmap(graph_map, graph_size);
> - close(fd);
> - exit(1);
> }
>
> static void prepare_commit_graph_one(struct repository *r, const char
> *obj_dir)
> diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c
> new file mode 100644
> index 00..420851d0d2
> --- /dev/null
> +++ b/fuzz-commit-graph.c
> @@ -0,0 +1,18 @@
> +#include "object-store.h"
> +#include "commit-graph.h"
> +
> +struct commit_graph *parse_commit_graph(void *graph_map, int fd,
> + size_t graph_size);
> +
> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
> +
> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
> +{
> + struct commit_graph *g;
> +
> + g = parse_commit_graph((void *) data, -1, size);
> + if (g)
> + free(g);
> +
> + return 0;
> +}
Re: [PATCH v3] list-objects.c: don't segfault for missing cmdline objects
Matthew DeVore writes: > When a command is invoked with both --exclude-promisor-objects, > --objects-edge-aggressive, and a missing object on the command line, > the rev_info.cmdline array could get a NULL pointer for the value of > an 'item' field. Prevent dereferencing of a NULL pointer in that > situation. > > Properly handle --ignore-missing. If it is not passed, die when an > object is missing. Otherwise, just silently ignore it. > > Signed-off-by: Matthew DeVore Thanks for an update. Will replace. > --- > revision.c | 2 ++ > t/t0410-partial-clone.sh | 16 ++-- > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/revision.c b/revision.c > index 13e0519c02..293303b67d 100644 > --- a/revision.c > +++ b/revision.c > @@ -1729,6 +1729,8 @@ int handle_revision_arg(const char *arg_, struct > rev_info *revs, int flags, unsi > if (!cant_be_filename) > verify_non_filename(revs->prefix, arg); > object = get_reference(revs, arg, , flags ^ local_flags); > + if (!object) > + return revs->ignore_missing ? 0 : -1; > add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags); > add_pending_object_with_path(revs, object, arg, oc.mode, oc.path); > free(oc.path); > diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh > index ba3887f178..169f7f10a7 100755 > --- a/t/t0410-partial-clone.sh > +++ b/t/t0410-partial-clone.sh > @@ -349,7 +349,7 @@ test_expect_success 'rev-list stops traversal at promisor > commit, tree, and blob > grep $(git -C repo rev-parse bar) out # sanity check that some walking > was done > ' > > -test_expect_success 'rev-list accepts missing and promised objects on > command line' ' > +test_expect_success 'rev-list dies for missing objects on cmd line' ' > rm -rf repo && > test_create_repo repo && > test_commit -C repo foo && > @@ -366,7 +366,19 @@ test_expect_success 'rev-list accepts missing and > promised objects on command li > > git -C repo config core.repositoryformatversion 1 && > git -C repo config extensions.partialclone "arbitrary string" && > - git -C repo rev-list --exclude-promisor-objects --objects "$COMMIT" > "$TREE" "$BLOB" > + > + for OBJ in "$COMMIT" "$TREE" "$BLOB"; do > + test_must_fail git -C repo rev-list --objects \ > + --exclude-promisor-objects "$OBJ" && > + test_must_fail git -C repo rev-list --objects-edge-aggressive \ > + --exclude-promisor-objects "$OBJ" && > + > + # Do not die or crash when --ignore-missing is passed. > + git -C repo rev-list --ignore-missing --objects \ > + --exclude-promisor-objects "$OBJ" && > + git -C repo rev-list --ignore-missing --objects-edge-aggressive > \ > + --exclude-promisor-objects "$OBJ" > + done > ' > > test_expect_success 'gc repacks promisor objects separately from > non-promisor objects' '
Re: git, monorepos, and access control
Jeff King writes: > In my opinion this feature is so contrary to Git's general assumptions > that it's likely to create a ton of information leaks of the supposedly > protected data. > ... Yup, with s/implemented/designed/, I agree all you said here (snipped). > Sorry I don't have a more positive response. What you want to do is > perfectly reasonable, but I just think it's a mismatch with how Git > works (and because of the security impact, one missed corner case > renders the whole thing useless). Yup, again. Storing source files encrypted and decrypting with smudge filter upon checkout (and those without the access won't get keys and will likely to use sparse checkout to exclude these priviledged sources) is probably the only workaround that does not involve submodules. Viewing "diff" and "log -p" would still be a challenge, which probably could use the same filter as smudge for textconv. I wonder (and this is the primary reason why I am responding to you) if it is common enough wish to use the same filter for smudge and textconv? So far, our stance (which can be judged from the way the clean/smudge filters are named) has been that the in-repo representation is the canonical, and the representation used in the checkout is ephemeral, and that is why we run "diff", "grep", etc. over the in-repo representation, but the "encrypted in repo, decrypted in checkout" abuse would be helped by an option to do the reverse---find changes and look substrings in the representation used in the checkout. I am not sure if there are other use cases that is helped by such an option.
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Frank Schäfer writes: > Just to be sure that I'm not missing anything here: > What's your definition of "LF in repository, CRLF in working tree" in > terms of config parameters ? :::Documentation/config/core.txt::: core.autocrlf:: Setting this variable to "true" is the same as setting the `text` attribute to "auto" on all files and core.eol to "crlf". Set to true if you want to have `CRLF` line endings in your working directory and the repository has LF line endings. This variable can be set to 'input', in which case no output conversion is performed.
Re: gitweb: local configuration not found
Ævar Arnfjörð Bjarmason writes:
>> Documentation says "If you are absolutely certain that you want your
>> script to load and execute a file from the current directory, then use
>> a ./ prefix". We can do that, like so:
>>
>> diff --git i/gitweb/Makefile w/gitweb/Makefile
>> index cd194d057f..3160b6cc5d 100644
>> --- i/gitweb/Makefile
>> +++ w/gitweb/Makefile
>> @@ -18,7 +18,7 @@ RM ?= rm -f
>> INSTALL ?= install
>>
>> # default configuration for gitweb
>> -GITWEB_CONFIG = gitweb_config.perl
>> +GITWEB_CONFIG = ./gitweb_config.perl
>> GITWEB_CONFIG_SYSTEM = /etc/gitweb.conf
>> GITWEB_CONFIG_COMMON = /etc/gitweb-common.conf
>> GITWEB_HOME_LINK_STR = projects
>>
>> but that does not help if someone overrides GITWEB_CONFIG, and besides,
>> it would be nicer to avoid the possibility of an @INC search altogether.
>> ...
> Just:
>
> local @INC = '.';
> do 'FILE.pl';
>
> Would do the same thing, but seems like a more indirect way to do it if
> all we want is ./ anyway.
Yeah, it does look indirect. Despite what you said, it also would
support users giving an absolute path via GITWEB_CONFIG.
With "use File::Spec", perhaps something like this?
gitweb/gitweb.perl | 4
1 file changed, 4 insertions(+)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 2594a4badb..239e7cbc25 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -719,6 +719,10 @@ sub filter_and_validate_refs {
sub read_config_file {
my $filename = shift;
return unless defined $filename;
+
+ $filename = File::Spec->catfile(".", $filename)
+ unless File::Spec->file_name_is_absolute($filename);
+
# die if there are errors parsing config file
if (-e $filename) {
do $filename;
Re: Any way to make git-log to enumerate commits?
Konstantin Khomoutov writes: > On Wed, Dec 05, 2018 at 05:22:14PM +0300, Konstantin Kharlamov wrote: > >> It would be great if git-log has a formatting option to insert an >> index of the current commit since HEAD. >> >> It would allow after quitting the git-log to immediately fire up "git >> rebase -i HEAD~index" instead of "git rebase -i >> go-copy-paste-this-long-number-id". > > This may have little sense in a general case as the history maintained > by Git is a graph, not a single line. Hence your prospective approach > would only work for cases like `git log` called with the > "--first-parent" command-line option. I do not see why the "name each rev relative to HEAD" formatting option cannot produce HEAD^2~2 etc. It would be similar to "git log | git name-rev --stdin" but I do not offhand recall if we had a way to tell name-rev to use only HEAD as the anchoring point.
Re: [RFC PATCH 3/3] test-lib: add the '--stress' option to run a test repeatedly under load
Jeff King writes: > But the ^C case is interesting. Try running your script under "sh -x" > and hitting ^C. The signal interrupts the first wait. In my script (or > yours modified to use a single wait), we then proceed immediately to the > "exit", and get output from the subshells after we've exited. > > But in your script with the loop, the first wait is interrupted, and > then the _second_ wait (in the second loop iteration) picks up all of > the exiting processes. The subsequent waits are all noops that return > immediately, because there are no processes left. > > So the right number of waits is either "1" or "2". Looping means we do > too many (which is mostly a harmless noop) or too few (in the off chance > that you have only a single job ;) ). So it works out in practice. Well, if you time your ^C perfectly, no number of waits is right, I am afraid. You spawn N processes and while looping N times waiting for them, you can ^C out of wait before these N processes all die, no? > But I think a more clear way to do it is to simply wait in the signal > trap, to cover the case when our original wait was aborted. Sounds good.
Re: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()
Jonathan Tan writes:
> Looking at the bigger picture, the speed of the connectivity check
> during a fetch might be further improved by passing only the negotiation
> tips (obtained through --negotiation-tip) instead of "--all". This patch
> just handles the low-hanging fruit first.
That sounds like a good direction, when having to list excessive
number of refs is the primary problem. When fetching their 'master'
into our 'remotes/origin/master' and doing nothing else, we may end
up showing only the latter, which will miss optimization opportunity
a lot if the latest change made over there is to merge in the change
we asked them to pull earlier (which would be greatly helped if we
let them know about the tip of the topic they earlier pulled from
us), but also avoids having to send irrelevant refs that point at
tags addded to months old states. So there is a subtle trade-off
between sending more refs to reduce the resulting packfile, and
sending fewer refs to reduce the cost of the "have" exchange.
Changing the way to throw each object pointed at by a ref into the
queue to be emitted in the "have" exchange from regular object
parsing to peeking of precomputed data would reduce the local cost
of "have" exchange, but it does not reduce the network cost at all,
though.
As to the change being specific to get_reference() and not to
parse_object(), I think what we see here is probably better, simply
because parse_object() is not in the position to asssume that it is
likely to be asked to parse commits, but I think get_reference() is,
after looking at its callsites in revision.c.
I do share the meta-comment concern with Peff, though.
> ---
> revision.c | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/revision.c b/revision.c
> index b5108b75ab..e7da2c57ab 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -212,7 +212,20 @@ static struct object *get_reference(struct rev_info
> *revs, const char *name,
> {
> struct object *object;
>
> - object = parse_object(revs->repo, oid);
> + /*
> + * If the repository has commit graphs, repo_parse_commit() avoids
> + * reading the object buffer, so use it whenever possible.
> + */
> + if (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {
> + struct commit *c = lookup_commit(revs->repo, oid);
> + if (!repo_parse_commit(revs->repo, c))
> + object = (struct object *) c;
> + else
> + object = NULL;
> + } else {
> + object = parse_object(revs->repo, oid);
> + }
> +
> if (!object) {
> if (revs->ignore_missing)
> return object;
Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences
Johannes Sixt writes: > Am 05.12.18 um 16:37 schrieb Elijah Newren: >> On Tue, Dec 4, 2018 at 11:40 PM Junio C Hamano wrote: >>> >>> Johannes Sixt writes: >>> >>>> Please let me deposit my objection. This paragraph is not the right >>>> place to explain what directory renme detection is and how it works >>>> under the hood. "works fine" in the original text is the right phrase >>>> here; if there is concern that this induces expectations that cannot >>>> be met, throw in the word "heuristics". >>>> >>>> Such as: >>>> Directory rename heuristics work fine in the merge and interactive >>>> backends. It does not in the am backend because... >>> >>> OK, good enough, I guess. Or just s/work fine/is enabled/. >> >> So... >> >> Directory rename heuristics are enabled in the merge and interactive >> backends. They are not in the am backend because it operates on >> "fake ancestors" that involve trees containing only the files modified >> in the patch. Due to the lack of accurate tree information, directory >> rename detection is disabled for the am backend. > > OK with me. And also if you drop sentence no.2 and keep just no.1 and no.3. Yeah, that shorter version may be sufficient to explain why we do not use the heuristics in the "am" backend.
Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences
Johannes Sixt writes: > Please let me deposit my objection. This paragraph is not the right > place to explain what directory renme detection is and how it works > under the hood. "works fine" in the original text is the right phrase > here; if there is concern that this induces expectations that cannot > be met, throw in the word "heuristics". > > Such as: >Directory rename heuristics work fine in the merge and interactive >backends. It does not in the am backend because... OK, good enough, I guess. Or just s/work fine/is enabled/.
Re: [PATCH v3 07/14] checkout: split into switch-branch and restore-files
Elijah Newren writes: > What depends on stage#2 coming from the commit that will become the > first parent? How about "git diff --cc" for a starter? What came from HEAD's ancestry should appear first and then what came from the side branch that is merged into.
Re: [WIP RFC 3/5] upload-pack: refactor reading of pack-objects out
Jonathan Tan writes:
> +struct output_state {
> + char buffer[8193];
> + int used;
> +};
> +
> +static int read_pack_objects_stdout(int outfd, struct output_state *os)
> +{
The naming feels quite odd. We are downstream of pack-objects and
reading its standard output stream, so "read stdout-of-pack-objects"
is not wrong per-se, but it may be just me. Stepping back and what
the function is really about often helps us to understand what we
are doing.
I think the function you extracted from the existing logic is
responsible for relaying the data read from pack-objects to the
fetch-pack client on the other side of the wire. So from that point
of view, singling out "read" as if it is a primary thing the
function does is already suboptimal. Perhaps
static int relay_pack_data(int in, struct output_state *os)
because the fd is what we "read" from (hence, 'in', not 'out'), and
relaying is what we do and reading is only one half of doing so?
> + /* Data ready; we keep the last byte to ourselves
> + * in case we detect broken rev-list, so that we
> + * can leave the stream corrupted. This is
> + * unfortunate -- unpack-objects would happily
> + * accept a valid packdata with trailing garbage,
> + * so appending garbage after we pass all the
> + * pack data is not good enough to signal
> + * breakage to downstream.
> + */
Yeah, I recall writing this funky and unfortunate logic in 2006.
Perhaps we want to update the comment style to make it a bit more
modern?
The "Data ready;" part of the comment applies more to what the
caller of this logic does (i.e. poll returned and revents indicates
we can read, and that is why we are calling into this logic). The
remainder is the comment that is relevat to this logic.
> + ssize_t readsz;
> +
> + readsz = xread(outfd, os->buffer + os->used,
> +sizeof(os->buffer) - os->used);
So we read what we could read in the remaining buffer.
I notice that you alloated 8k+1 bytes; would this code end up
reading that 8k+1 bytes in full under the right condition? I am
mainly wondering where the need for +1 comes from.
> + if (readsz < 0) {
> + return readsz;
> + }
OK, report an error to the caller by returning negative. I am
guessing that you'd have more code in this block that currently have
only one statement in the future steps, perhaps.
> + os->used += readsz;
> +
> + if (os->used > 1) {
> + send_client_data(1, os->buffer, os->used - 1);
> + os->buffer[0] = os->buffer[os->used - 1];
OK, this corresponds to the "*cp++ = buffered" in the original just
before xread().
> + os->used = 1;
> + } else {
> + send_client_data(1, os->buffer, os->used);
> + os->used = 0;
I am not sure if the code is correct when os->used happens to be 1
(shouldn't we hold the byte, skip the call to send_client_data(),
and go back to poll() to expect more data?), but this is a faithful
code movement and rewrite of the original.
I've read the caller (below, snipped) and the conversion looked
sensible there as well. The final flushing of the byte(s) held back
in *os also looks good.
Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc
Junio C Hamano writes: > So, this is OK, but > >> +Clients then should understand that the returned packfile could be >> incomplete, >> +and that it needs to download all the given URIs before the fetch or clone >> is >> +complete. Each URI should point to a Git packfile (which may be a thin pack >> and >> +which may contain offset deltas). > > weaken or remove the (parenthetical comment) in the last sentence, > and replace the beginning of the section with something like > > If the client replies with 'packfile-uris', when the server > sends the packfile, it MAY send a `packfile-uris` section... > > You may steal what I wrote in the above response to help the > server-side folks to decide how to actually implement the "it MAY > send a packfile-uris" part in the document. By the way, I do agree with the practical consideration the design you described makes. For a pregenerated pack that brings you from v1.0 to v2.0, "thin" would roughly save the transfer of one full checkout (compressed, of course), and "ofs" would also save several bytes per object. Compared to a single pack that delivers everything the fetcher wants, concatenation of packs without "thin" to transfer the same set of objects would cost quite a lot more. And I do not think we should care too much about fetchers that lack either thin or ofs, so I'd imagine that any client that ask for packfile-uris would also advertise thin and ofs as well, so in practice, a request with packfile-uris that lack thin or ofs would be pretty rare and requiring all three and requiring only one would not make much practical difference. It's just that I think singling out these two capabilities as hard requirements at the protocol level is wrong.
Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc
Jonathan Tan writes: > +This feature allows servers to serve part of their packfile response as URIs. > +This allows server designs that improve scalability in bandwidth and CPU > usage > +(for example, by serving some data through a CDN), and (in the future) > provides > +some measure of resumability to clients. Without reading the remainder, this makes readers anticipate a few good things ;-) - "part of", so pre-generated constant material can be given from CDN and then followed-up by "filling the gaps" small packfile, perhaps? - The "part of" transmission may not bring the repository up to date wrt to the "want" objects; would this feature involve "you asked history up to these commits, but with this pack-uri, you'll be getting history up to these (somewhat stale) commits"? Anyway, let's read on. > +This feature is available only in protocol version 2. > + > +Protocol > + > + > +The server advertises `packfile-uris`. > + > +If the client replies with the following arguments: > + > + * packfile-uris > + * thin-pack > + * ofs-delta "with the following" meaning "with all of the following", or "with any of the following"? Is there a reason why the server side must require that the client understands and is willing to accept a thin-pack when wanting to use packfile-uris? The same question for the ofs-delta. When the pregenerated constant material the server plans to hand out the uris for was prepared by using ofs-delta encoding, the server cannot give the uri to it when the client does not want ofs-delta encoded packfile, but it feels somewhat strange that we require the most capable client at the protocol level. After all, the server side could prepare one with ofs-delta and another without ofs-delta and depending on what the client is capable of, hand out different URIs, if it wanted to. The reason why I care is because thin and ofs will *NOT* stay forever be the only optional features of the pack format. We may invent yet another such optional 'frotz' feature, which may greatly help the efficiency of the packfile encoding, hence it may be preferrable to always generate a CDN packfile with that feature, in addition to thin and ofs. Would we add 'frotz' to the above list in the documentation, then? What would happen to existing servers and clients written before that time then? My recommendation is to drop the mention of "thin" and "ofs" from the above list, and also from the following paragraph. The "it MAY send" will serve as a practical escape clause to allow a server/CDN implementation that *ALWAYS* prepares pregenerated material that can only be digested by clients that supports thin and ofs. Such a server can send packfile-URIs only when all of the three are given by the client and be compliant. And such an update to the proposed document would allow a more diskful server to prepare both thin and non-thin pregenerated packs and choose which one to give to the client depending on the capability. > +when the server sends the packfile, it MAY send a `packfile-uris` section > +directly before the `packfile` section (right after `wanted-refs` if it is > +sent) containing HTTP(S) URIs. See protocol-v2.txt for the documentation of > +this section. So, this is OK, but > +Clients then should understand that the returned packfile could be > incomplete, > +and that it needs to download all the given URIs before the fetch or clone is > +complete. Each URI should point to a Git packfile (which may be a thin pack > and > +which may contain offset deltas). weaken or remove the (parenthetical comment) in the last sentence, and replace the beginning of the section with something like If the client replies with 'packfile-uris', when the server sends the packfile, it MAY send a `packfile-uris` section... You may steal what I wrote in the above response to help the server-side folks to decide how to actually implement the "it MAY send a packfile-uris" part in the document. > +Server design > +- > + > +The server can be trivially made compatible with the proposed protocol by > +having it advertise `packfile-uris`, tolerating the client sending > +`packfile-uris`, and never sending any `packfile-uris` section. But we should > +include some sort of non-trivial implementation in the Minimum Viable > Product, > +at least so that we can test the client. > + > +This is the implementation: a feature, marked experimental, that allows the > +server to be configured by one or more `uploadpack.blobPackfileUri= > +` entries. Whenever the list of objects to be sent is assembled, a blob > +with the given sha1 can be replaced by the given URI. This allows, for > example, > +servers to delegate serving of large blobs to CDNs. ;-) > +Client design > +- > + > +While fetching, the client needs to remember the list of URIs and cannot > +declare that the fetch is complete until all URIs have been downloaded as > +packfiles. > + > +The division
Re: [WIP RFC 1/5] Documentation: order protocol v2 sections
Jonathan Tan writes: > The git command line expects Git servers to follow a specific order of "Command line"? It sounds like you are talking about the order of command line arguments and options, but apparently that is not what you are doing. Is it "The git over-the-wire protocol"? > +output = acknowledgements flush-pkt | > + [acknowledgments delim-pkt] [shallow-info delim-pkt] > + [wanted-refs delim-pkt] packfile flush-pkt So the output can be either - acks followed by flush (and nothing else) or - (possibly) acks, followed by (possibly) shallow, followed by (possibly) wanted-refs, followed by the pack stream and flush at the end. > @@ -335,9 +335,10 @@ header. > *PKT-LINE(%x01-03 *%x00-ff) > > acknowledgments section > - * If the client determines that it is finished with negotiations > - by sending a "done" line, the acknowledgments sections MUST be > - omitted from the server's response. > + * If the client determines that it is finished with negotiations by > + sending a "done" line (thus requiring the server to send a packfile), > + the acknowledgments sections MUST be omitted from the server's > + response. OK. > * Always begins with the section header "acknowledgments" > > @@ -388,9 +389,6 @@ header. > which the client has not indicated was shallow as a part of > its request. > > - * This section is only included if a packfile section is also > - included in the response. > - Earlier, we said that shallow-info is not given when packfile is not there. That is captured in the updated EBNF above. We don't have a corresponding removal of a bullet point for wanted-refs section below but probably that is because the original did not have corresponding bullet point to begin with. > wanted-refs section > * This section is only included if the client has requested a > ref using a 'want-ref' line and if a packfile section is also
Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences
Elijah Newren writes:
> Gah, when I was rebasing on your patch I adopted your sentence rewrite
> but forgot to remove the "sometimes". Thanks for catching; correction:
>
> -- 8< --
> Subject: [PATCH v2] git-rebase.txt: update note about directory rename
> detection and am
>
> In commit 6aba117d5cf7 ("am: avoid directory rename detection when
> calling recursive merge machinery", 2018-08-29), the git-rebase manpage
> probably should have also been updated to note the stronger
> incompatibility between git-am and directory rename detection. Update
> it now.
>
> Signed-off-by: Elijah Newren
> ---
> Documentation/git-rebase.txt | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 41631df6e4..ef76cccf3f 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -569,8 +569,12 @@ it to keep commits that started empty.
> Directory rename detection
> ~~
>
> -The merge and interactive backends work fine with
> -directory rename detection. The am backend sometimes does not.
> +The merge and interactive backends work fine with directory rename
I am not sure "work fine" a fair and correct label, as rename is
always heuristic.
The "directory rename detection" heuristic in "merge" and the
"interactive" backends can take what happened to paths in the
same directory into account when deciding if a disappeared path
was "renamed" and to which other path. The heuristic produces
incorrect result when the information given is only about
changed paths, which is why it is disabled when using the "am"
backend.
perhaps.
Re: [PATCH 3/3] sha1-file: change alternate "error:" message to "warning:"
Ævar Arnfjörð Bjarmason writes:
> Change the "error" message emitted by alt_odb_usable() to be a
> "warning" instead. As noted in commits leading up to this one this has
> never impacted the exit code ever since the check was initially added
> in 26125f6b9b ("detect broken alternates.", 2006-02-22).
>
> It's confusing to emit an "error" when e.g. "git fsck" will exit with
> 0, so let's emit a "warning:" instead.
OK, that sounds sensible. An alternative that may be more sensible
is to actually diagnose this as an error, but the purpose of this
message is to help users diagnose a possible misconfiguration and
keeping the repository "working" with the remaining set of object
stores by leaving it as a mere warning, like this patch does, is
probably a better approach.
Re: [PATCH 2/3] sha1-file: emit error if an alternate looks like a repository
Ævar Arnfjörð Bjarmason writes:
> Since 26125f6b9b ("detect broken alternates.", 2006-02-22) we've
> emitted an error if the alternates directory doesn't exist, but not
> for the common misstep of adding a path to another git repository as
> an alternate, as opposed to its "objects" directory.
>
> Let's check for this, i.e. whether X/objects or X/.git/objects exists
> if the user supplies X and print an error (which as a commit leading
> up to this one shows doesn't change the exit code, just "warns").
I agree that "Let's check for this" is a good idea, but do not
necessarily agree with "i.e.". Don't we have a helper that takes
the path to an existing directory and answers "Yup, it does look
like a Git repository"? Using that is a lot more in line with what
you claimed to do in the title for this patch.
I haven't read 3/3 yet, but as I said, I suspect it is reasonable to
DWIM and use the object store associated with the directory we found
to be a repository.
Re: [PATCHv2 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip
o know whether we need to try fetching
the
@@ -29,6 +24,10 @@
function check_for_new_submodule_commits(), so we'll also run that code
in case the submodule recursion is set to "on".
+The submodule checks were done only when a ref in the superproject
+changed, these checks were extended to also be performed when fetching
+into FETCH_HEAD for completeness, and add a test for that too.
+
OK.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
@@ -41,30 +40,39 @@
...
The range-diff output for this step is unreadble for me, but the
code around this area does not seem to appear in the comparison
between the result of applying these directly to master and the
result of merging the previous round to master, so perhaps this is
just an indication that later follow-up fix has been squashed into
this step or something, which I shouldn't have to worry about.
diff --git a/submodule.c b/submodule.c
--- a/submodule.c
@@ -73,8 +81,10 @@
int result;
struct string_list changed_submodule_names;
-+ struct get_next_submodule_task **fetch_specific_oids;
-+ int fetch_specific_oids_nr, fetch_specific_oids_alloc;
++
++ /* The submodules to fetch in */
++ struct fetch_task **oid_fetch_tasks;
++ int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
OK. The task struct has been renamed and the new name makes more
sense ("getting the next submodule" is less important than "what we
are going to do to that submodule").
...
-+struct get_next_submodule_task {
++struct fetch_task {
+ struct repository *repo;
+ const struct submodule *sub;
+ unsigned free_sub : 1; /* Do we need to free the submodule? */
...
+ return (const struct submodule *) ret;
+}
+
-+static struct get_next_submodule_task *get_next_submodule_task_create(
-+ struct repository *r, const char *path)
++static struct fetch_task *fetch_task_create(struct repository *r,
++ const char *path)
+{
-+ struct get_next_submodule_task *task = xmalloc(sizeof(*task));
++ struct fetch_task *task = xmalloc(sizeof(*task));
+ memset(task, 0, sizeof(*task));
+
+ task->sub = submodule_from_path(r, _oid, path);
+ if (!task->sub) {
-+ task->sub = get_default_submodule(path);
++ /*
++ * No entry in .gitmodules? Technically not a submodule,
++ * but historically we supported repositories that happen to be
++ * in-place where a gitlink is. Keep supporting them.
++ */
++ task->sub = get_non_gitmodules_submodule(path);
++ if (!task->sub) {
++ free(task);
++ return NULL;
++ }
++
+ task->free_sub = 1;
OK.
-+ if (!task->sub) {
-+ free(task);
+- }
++ task = fetch_task_create(spf->r, ce->name);
++ if (!task)
+ continue;
- }
OK, so the code used to signal the need to work with the presense of
task->sub but now task's NULLness is used, so no need to free.
@@ -231,24 +253,26 @@
+ return 1;
} else {
+
-+ get_next_submodule_task_release(task);
++ fetch_task_release(task);
+ free(task);
+
OK.
-+ /* NEEDSWORK: have get_default_remote from s--h */
++ /* NEEDSWORK: have get_default_remote from submodule--helper */
;-)
Re: [PATCH v3 07/14] checkout: split into switch-branch and restore-files
Duy Nguyen writes: > On Tue, Dec 4, 2018 at 6:43 PM Elijah Newren wrote: >> > > > +--ours:: >> > > > +--theirs:: >> ... >> go away. Maybe it can still be fixed (I haven't dug too deeply into >> it), but if so, the only fix needed here would be to remove this long >> explanation about why the tool gets things totally backward. > > Aha. I' not really deep in this merge business to know if stages 2 and > 3 can be swapped. This is right up your alley. I'll just leave it to > you. Please don't show stage#2 and stage#3 swapped to the end user, unless that is protected behind an option (not per-repo config). It is pretty much ingrained that stage#2 is what came from the commit that will become the first parent of the commit being prepared, and changing it without an explicit command line option will break tools. > I'm actually still not sure how to move it here (I guess 'here' is > restore-files since we won't move HEAD). All the --mixed, --merge and > --hard are confusing. But maybe we could just make 'git restore-files > --from HEAD -f :/" behave just like "git reset --hard HEAD" (but with > some safety net) But we can leave it for discussion in the next round. Perhaps you two should pay a bit closer attention to what Thomas Gummerer is working on. I've touched above in my earlier comments, too, e.g.
Re: [PATCH v3 07/14] checkout: split into switch-branch and restore-files
Duy Nguyen writes: >> My single biggest worry about this whole series is that I'm worried >> you're perpetuating and even further ingraining one of the biggest >> usability problems with checkout: people suggest and use it for >> reverting/restoring paths to a previous version, but it doesn't do >> that: > > ... > > git restore-files --from=master~10 Documentation/ The "single biggest worry" could be due to Elijah not being aware of other recent discussions. My understanding of the plan is - "git checkout" will learn a new "--[no-]overlay" option, where the current behaviour, i.e. "take paths in master~10 that match pathspec Documentation/, and overlay them on top of what is in the index and the working tree", is explained as "the overlay mode" and stays to be the default. With "checkout --no-overlay master~10 Documentation/", the command will become "replace paths in the current index and the working tree that match the pathspec Documentation/ with paths in master~10 that match pathspec Documentation/". - "git restore-files --from= " by default will use "--no-overlay" semantics, but the users can still use "--overlay" from the command line as an option. So "restore-files" would become truly "restore the state of Documentation/ to match that of master~10", I would think. >> Also, the fact that we're trying to make a simpler command makes me >> think that removing the auto-vivify behavior from the default and >> adding a simple flag which users can pass to request will allow this >> part of the documentation to be hidden behind the appropriate flag, >> which may make it easier for users to compartmentalize the command and >> it's options, enabling them to learn as they go. > > Sounds good. I don't know a good name for this new option though so > unless anybody comes up with some suggestion, I'll just disable > checkout.defaultRemote in switch-branch. If it comes back as a new > option, it can always be added later. Are you two discussing the "checkout --guess" option? I am somewhat lost here. >> > +-f:: >> > +--force:: >> > + Proceed even if the index or the working tree differs from >> > + HEAD. This is used to throw away local changes. >> >> Haven't thought through this thoroughly, but do we really need an >> option for that instead of telling users to 'git reset --hard HEAD' >> before switching branches if they want their stuff thrown away? > > For me it's just a bit more convenient. Hit an error when switching > branch? Recall the command from bash history, stick -f in it and run. > Elsewhere I think both Junio and Thomas (or maybe only Junio) suggests > moving the "git reset" functionality without moving HEAD to one of > these commands, which goes the opposite direction... Isn't there a huge difference? "checkout --force " needs to clobber only the changes that are involved in the switch, i.e. if your README.txt is the same between master and maint while Makefile is different, after editing both files while on master, you can not "switch-branch" to maint without doing something to Makefile (i.e. either discard your local change or wiggle your local change to the context of 'maint' with "checkout -m"). But you can carry the changes to README.txt while checking out 'maint' branch. Running "git reset --hard HEAD" would mean that you will lose the changes to README.txt as well. >> > +--orphan :: >> > + Create a new 'orphan' branch, named , started from >> > +and switch to it. The first commit made on this >> >> What?? started from ? The whole point of --orphan is >> you have no parent, i.e. no start point. Also, why does the >> explanation reference an argument that wasn't in the immediately >> preceding synopsis? > > I guess bad phrasing. It should be "switch to first, > then prepare the worktree so that the first commit will have no > parent". Or something along that line. It should be a , no? It is not a "point" in history, but is "start with this tree". I may have more comments on this message but that's it from me for now.
Re: [PATCH v3 07/14] checkout: split into switch-branch and restore-files
Elijah Newren writes: >> +Updates files in the working tree to match the version in the index >> +or the specified tree. >> + >> +'git restore-files' [--from=] ...:: > > and ? I understand and , > or but have no clue why it'd be okay to specify > and together. What does that even mean? I have this tree object v2.6.11-tree that is not enclosed in a commit object. I want to take the top-level Makefile out of that tree, stuff it in the index and overwrite the working tree file. $ git checkout v2.6.11-tree Makefile $ git restore-files --from=v2.6.11-tree Makefile >> + Overwrite paths in the working tree by replacing with the >> + contents in the index or in the (most often a >> + commit). When a is given, the paths that >> + match the are updated both in the index and in >> + the working tree. > > Is that the default we really want for this command? Why do we > automatically assume these files are ready for commit? I understand > that it's what checkout did, but I'd find it more natural to only > affect the working tree by default. We can give it an option for > affecting the index instead (or perhaps in addition). Oooah. Now this is getting juicy. I do think supporting "--index" (which would make it more in line with what Duy wrote), with optionally "--cached" as well, and making the "working tree only" mode as default may not be a bad idea. I am offhand not sure how the "working tree only" mode (similar to the default mode of "git apply" that mimics the way "patch -p1" works) should interact with the non-overlay mode of the command, but other than that, I tend to agree with the idea that restore-files is only a part of making the contents into committable shape, not exactly ready for it yet.
Re: [PATCH] sideband: color lines with keyword only
Jonathan Nieder writes:
> Stefan Beller wrote:
>> On Mon, Dec 3, 2018 at 3:23 PM Jonathan Nieder wrote:
>
>>> I was curious about what versions of Gerrit this is designed to
>>> support (or in other words whether it's a bug fix or a feature).
Well, bf1a11f0 ("sideband: highlight keywords in remote sideband
output", 2018-08-07) clearly wanted to allow a keyword followed by
anything !isalnum() to be painted, and we accepted that change
because we thought it was a good idea, so anything that made a
keyword alone not to be painted is a bug, isn't it? Whether output
lines from Gerrit benefits from this fix is a different matter, of
course.
> No worries. Can't hurt for Junio to have a few patches to apply to
> "pu" or "next" to practice using the release candidates. :)
This change falls into "an obvious and small fix to a bug that went
unnoticed and is in an older release (2.19)" category, which is not
eligible for the upcoming release this late in the cycle. I think
enough eyeballs looked at the change already, so let's not waste the
already-spent review braincycle and mark it as "Will merge to 'next'".
Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences
Johannes Sixt writes: > Am 03.12.18 um 21:42 schrieb Martin Ågren: >> On Mon, 3 Dec 2018 at 18:35, Johannes Sixt wrote: >>> I actually did not test the result, because I don't have the >>> infrastructure. >> >> I've tested with asciidoc and Asciidoctor, html and man-page. Looks >> good. > > Thank you so much! > > -- Hannes Thanks, both.
Re: [RFC] git clean --local
Junio C Hamano writes: > If "git clean" takes a pathspec, perhaps you can give a negative > pathspec to exclude whatever you do not want to get cleaned, > something like > > git clean '*.o' ':!precious.o' > > to say "presious.o is ignored (hence normally expendable), but I do > not want to clean it with this invocation of 'git clean'"? Hmph, this leads me to an interesting thought. With today's code, these two commands behave in meaningfully different ways when I mark some paths that match .gitignore patterns with the precious attribute. echo "*.ignored" >>.git/info/exclude echo "precious.* precious" >>.git/info/attributes : >expendable.ignored 2>precious.ignored git clean -n -x git clean -n -x ':(exclude,attr:precious)' I am not suggesting that giving "git clean" a configuration knob that always append pathspec elements, which would allow users to use the mechanism to set the above magic pathspec, would be a good approach. If we were to follow through this line of thought, an obvious thing to do is to always unconditonally append the above magic pathspec internally when running "git clean", which would mean * Existing projects and users' repositories will see no behaviour change, because they are unaware of the "precious" attribute. * People who learn the new feature can start using the "ignored but precious" class, without any need for transition period.
Re: [PATCH 3/3] RelNotes 2.20: drop spurious double quote
Martin Ågren writes: > We have three double-quote characters, which is one too many or too few. > Dropping the last one seems to match the original intention best. Thanks for spotting. The actual original intention was that the user says two things: first saying "add only what does not match '*' out of all branches" and then saying "add all branches, without any exclusion this time". But letting the user first say one thing and then doing another thing without saying it is also fine, which is what your version is. > > Signed-off-by: Martin Ågren > --- > Documentation/RelNotes/2.20.0.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/RelNotes/2.20.0.txt > b/Documentation/RelNotes/2.20.0.txt > index 201135d80c..e71fe3dee1 100644 > --- a/Documentation/RelNotes/2.20.0.txt > +++ b/Documentation/RelNotes/2.20.0.txt > @@ -578,7 +578,7 @@ Fixes since v2.19 > > * "git rev-parse --exclude=* --branches --branches" (i.e. first > saying "add only things that do not match '*' out of all branches" > - and then adding all branches, without any exclusion this time") > + and then adding all branches, without any exclusion this time) > worked as expected, but "--exclude=* --all --all" did not work the > same way, which has been fixed. > (merge 5221048092 ag/rev-parse-all-exclude-fix later to maint).
Re: [PATCH 2/3] RelNotes 2.20: clarify sentence
Martin Ågren writes: > I had to read this sentence a few times to understand it. Let's try to > clarify it. Great. Thanks. > > Signed-off-by: Martin Ågren > --- > Documentation/RelNotes/2.20.0.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/RelNotes/2.20.0.txt > b/Documentation/RelNotes/2.20.0.txt > index e5ab8cc609..201135d80c 100644 > --- a/Documentation/RelNotes/2.20.0.txt > +++ b/Documentation/RelNotes/2.20.0.txt > @@ -305,7 +305,7 @@ Performance, Internal Implementation, Development Support > etc. > > * The overly large Documentation/config.txt file have been split into > million little pieces. This potentially allows each individual piece > - included into the manual page of the command it affects more easily. > + to be included into the manual page of the command it affects more easily. > > * Replace three string-list instances used as look-up tables in "git > fetch" with hashmaps.
Re: [PATCH 1/3] RelNotes 2.20: move some items between sections
Martin Ågren writes: > Some items that should be in "Performance, Internal Implementation, > Development Support etc." have ended up in "UI, Workflows & Features" > and "Fixes since v2.19". Move them, and do s/uses/use/ while at it. > > Signed-off-by: Martin Ågren > --- I agree with the early half of this change; I think it is OK to consider lack of preparation for Travis transition and lack of better testing in the maintenance track as bugs, though. > Documentation/RelNotes/2.20.0.txt | 26 +- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/Documentation/RelNotes/2.20.0.txt > b/Documentation/RelNotes/2.20.0.txt > index b1deaf37da..e5ab8cc609 100644 > --- a/Documentation/RelNotes/2.20.0.txt > +++ b/Documentation/RelNotes/2.20.0.txt > @@ -137,11 +137,6 @@ UI, Workflows & Features > command line, or setting sendemail.suppresscc configuration > variable to "misc-by", can be used to disable this behaviour. > > - * Developer builds now uses -Wunused-function compilation option. > - > - * One of our CI tests to run with "unusual/experimental/random" > - settings now also uses commit-graph and midx. > - > * "git mergetool" learned to take the "--[no-]gui" option, just like > "git difftool" does. > > @@ -185,6 +180,11 @@ UI, Workflows & Features > > Performance, Internal Implementation, Development Support etc. > > + * Developer builds now use -Wunused-function compilation option. > + > + * One of our CI tests to run with "unusual/experimental/random" > + settings now also uses commit-graph and midx. > + > * When there are too many packfiles in a repository (which is not > recommended), looking up an object in these would require > consulting many pack .idx files; a new mechanism to have a single > @@ -387,6 +387,14 @@ Performance, Internal Implementation, Development > Support etc. > two classes to ease code migration process has been proposed and > its support has been added to the Makefile. > > + * The "container" mode of TravisCI is going away. Our .travis.yml > + file is getting prepared for the transition. > + (merge 32ee384be8 ss/travis-ci-force-vm-mode later to maint). > + > + * Our test scripts can now take the '-V' option as a synonym for the > + '--verbose-log' option. > + (merge a5f52c6dab sg/test-verbose-log later to maint). > + > > Fixes since v2.19 > - > @@ -544,14 +552,6 @@ Fixes since v2.19 > didn't make much sense. This has been corrected. > (merge 669b1d2aae md/exclude-promisor-objects-fix later to maint). > > - * The "container" mode of TravisCI is going away. Our .travis.yml > - file is getting prepared for the transition. > - (merge 32ee384be8 ss/travis-ci-force-vm-mode later to maint). > - > - * Our test scripts can now take the '-V' option as a synonym for the > - '--verbose-log' option. > - (merge a5f52c6dab sg/test-verbose-log later to maint). > - > * A regression in Git 2.12 era made "git fsck" fall into an infinite > loop while processing truncated loose objects. > (merge 18ad13e5b2 jk/detect-truncated-zlib-input later to maint).
Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed
Jeff King writes:
> That said, our C99 designated initializer weather-balloons haven't
> gotten any complaints yet. So I think you could actually do:
>
> struct setup_revision_opt s_r_opt = {
> .allow_exclude_promisor_objects = 1,
> };
> ...
> setup_revisions(...);
>
> which is pretty nice.
Yup. The output from
$ git grep -n ' \.[a-z0-9_]* =' -- \*.[ch]
with a bit of "git blame" tells us that cbc0f81d ("strbuf: use
designated initializers in STRBUF_INIT", 2017-07-10) is the balloon
for this exact feature. The same for array was done in 512f41cf
("clean.c: use designated initializer", 2017-07-14)
[I am writing it down so that I do not have to dig for it every time
and instead can ask the list archive]
Re: [PATCH v3] range-diff: always pass at least minimal diff options
Eric Sunshine writes: > This is a re-roll of Martin's v2[1]. The only difference from v2 is that > it retains coloring when emitting to the terminal (plus an in-code > comment was simplified). > > The regression introduced by d8981c3f88, in which the range-diff only > ever gets emitted to the terminal, and never to the cover letter or > commentary section of a standalone patch, makes the --range-diff option > rather useless, so this fix probably ought to be fast-tracked. Yup. Thanks. The only thing that makes me wonder is why any of the existing tests (among which I think I saw range-diff driven from format-patch) did not catch this rather obvious glitch. > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > index e497c1358f..048feaf6dd 100755 > --- a/t/t3206-range-diff.sh > +++ b/t/t3206-range-diff.sh > @@ -248,18 +248,24 @@ test_expect_success 'dual-coloring' ' > for prev in topic master..topic > do > test_expect_success "format-patch --range-diff=$prev" ' > - git format-patch --stdout --cover-letter --range-diff=$prev \ > + git format-patch --cover-letter --range-diff=$prev \ > master..unmodified >actual && Ah, of course. Then the "actual" file gets the names of the output files; we expect to see 5 of them. But now we have lost all the range-diff tests run under "--stdout", so next time some other change regresses only that codepath, we will not notice (which is fine for now but would want to be addressed before the end of the year around which time we certainly will all forget). Thanks. > - grep "= 1: .* s/5/A" actual && > - grep "= 2: .* s/4/A" actual && > - grep "= 3: .* s/11/B" actual && > - grep "= 4: .* s/12/B" actual > + test_when_finished "rm 000?-*" && > + test_line_count = 5 actual && > + test_i18ngrep "^Range-diff:$" -* && > + grep "= 1: .* s/5/A" -* && > + grep "= 2: .* s/4/A" -* && > + grep "= 3: .* s/11/B" -* && > + grep "= 4: .* s/12/B" -* > ' > done > > test_expect_success 'format-patch --range-diff as commentary' ' > - git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual && > - test_i18ngrep "^Range-diff:$" actual > + git format-patch --range-diff=HEAD~1 HEAD~1 >actual && > + test_when_finished "rm 0001-*" && > + test_line_count = 1 actual && > + test_i18ngrep "^Range-diff:$" 0001-* && > + grep "> 1: .* new message" 0001-* > ' > > test_done
Re: [PATCH] t5004: avoid using tar for empty packages
Carlo Marcelo Arenas Belón writes:
> ea2d20d4c2 ("t5004: avoid using tar for checking emptiness of archive",
> 2013-05-09), introduced a fake empty tar archive to allow for portable
> tests of emptiness without having to invoke tar
>
> 4318094047 ("archive: don't add empty directories to archives", 2017-09-13)
> changed the expected result for its tests from one containing an empty
> directory to a plain empty archive but the portable test wasn't updated
> resulting on them failing again in (at least) NetBSD and OpenBSD
>
> Signed-off-by: Carlo Marcelo Arenas Belón
> ---
> t/t5004-archive-corner-cases.sh | 17 -
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
> index ced44355ca..271eb5a1fd 100755
> --- a/t/t5004-archive-corner-cases.sh
> +++ b/t/t5004-archive-corner-cases.sh
> @@ -3,8 +3,12 @@
> test_description='test corner cases of git-archive'
> . ./test-lib.sh
>
> -test_expect_success 'create commit with empty tree' '
> - git commit --allow-empty -m foo
> +# the 10knuls.tar file is used to test for an empty git generated tar
> +# without having to invoke tar because an otherwise valid empty GNU tar
> +# will be considered broken by {Open,Net}BSD tar
> +test_expect_success 'create commit with empty tree and fake empty tar' '
> + git commit --allow-empty -m foo &&
> + perl -e "print \"\\0\" x 10240" >10knuls.tar
> '
OK, so you moved the creation of the file with block of NULs to the
set-up phase of the entire script.
> # Make a dir and clean it up afterwards
> @@ -47,7 +51,6 @@ test_expect_success HEADER_ONLY_TAR_OK 'tar archive of
> commit with empty tree' '
>
> test_expect_success 'tar archive of empty tree is empty' '
> git archive --format=tar HEAD: >empty.tar &&
> - perl -e "print \"\\0\" x 10240" >10knuls.tar &&
> test_cmp_bin 10knuls.tar empty.tar
> '
And because of that, this one that was added for ea2d20d4 ("t5004:
avoid using tar for checking emptiness of archive", 2013-05-09) is
now simplified. It can use the one that was created in the set-up
phase.
> @@ -106,16 +109,12 @@ test_expect_success 'create a commit with an empty
> subtree' '
>
> test_expect_success 'archive empty subtree with no pathspec' '
> git archive --format=tar $root_tree >subtree-all.tar &&
> - make_dir extract &&
> - "$TAR" xf subtree-all.tar -C extract &&
> - check_dir extract
> + test_cmp_bin 10knuls.tar subtree-all.tar
> '
And then we avoid the test that assumes that an empty tar archive
can safely and portably extracted, and instead check the emptiness
the same way as the earlier test here ...
> test_expect_success 'archive empty subtree by direct pathspec' '
> git archive --format=tar $root_tree -- sub >subtree-path.tar &&
> - make_dir extract &&
> - "$TAR" xf subtree-path.tar -C extract &&
> - check_dir extract
> + test_cmp_bin 10knuls.tar subtree-path.tar
> '
... and here, too.
OK, and the result is consistent with the "We can help GNU and BSD
tar, but NetBSD tar cannot be salvageable" approach, laid out in the
earlier ea2d20d4 ("t5004: avoid using tar for checking emptiness of
archive", 2013-05-09). Makes sense.
Thanks.
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Frank Schäfer writes: > Hi Junio, > > Am 29.11.18 um 03:11 schrieb Junio C Hamano: > [...] >> This was misspoken a bit. Let's revise it to >> >> When producing a colored output (not limited to whitespace >> error coloring of diff output) for contents that are not >> marked as eol=crlf (and other historical means), insert >> before a CR that comes immediately before a LF. > You mean > ... > *after* a CR that comes immediately before a LF." > > OK, AFAICS this produces the desired output in all cases if eol=lf. OK, yeah, I think I meant "after", i.e. ... CR LF, in order to force CR to be separated from LF. > Now what about the case eol=crlf ? I have no strong opinions, other than that "LF in repository, CRLF in working tree" would make the issue go away (when it is solved for EOL=LF like the above, that is). > Keeping the current behavior of '-' lines is correct. > But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ? If "LF in repository, CRLF in working tree" is done, there would not be ^M at the end of the line, not just for removed lines, but also for added lines, no?
Re: [PATCH] Do not fail test if '.' is part of $PATH
"H.Merijn Brand" writes: > When $PATH contains the current directory as .:PATH, PATH:., PATH:.:PATH, > or (maybe worse) as :PATH, PATH:, or PATH::PATH - as an empty entry is > identical to having dot in $PATH - this test used to fail It is totally unclear what "this test" refers to. Let's retitle it to > Subject: [PATCH] t0061: do not fail test if '.' is part of $PATH and do something like this: t0061 created a script named with an unlikely name in the current directory to ensure that it is not found via the run_command() API, expecting that $PATH does not contain an element that names the current directory (i.e. '.' or '') in a sane environment. This obviously would not work if the $PATH does contain such an element. Introduce a DOT_IN_PATH lazy prerequisite to catch such a case and skip the test when the environment is not so sane. > +test_lazy_prereq DOT_IN_PATH ' > + case ":$PATH:" in > + *:.:*|*::*) true ;; > + *) false ;; > + esac > +' > + > +test_expect_success !DOT_IN_PATH 'run_command is restricted to PATH' ' > write_script should-not-run <<-\EOF && > echo yikes > EOF I also like Peff's more straight-forward approach that avoids looking into PATH but instead ask the shell what we care about (i.e. would we end up running 'should-not-run' if we asked the system to run it without giving an explicit path to it?). The last paragraph of the above would need to change if we were to go in that direction to something like Check if the running shell picks up the script without an explicit path to it and skip the test when it does. perhaps. The code to do so got a bit more compact than what Peff wrote but I think it still retains its main beauty, which is how straight-forward it is. t/t0061-run-command.sh | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index cf932c8514..17b560370e 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -29,7 +29,15 @@ test_expect_success 'run_command can run a command' ' test_must_be_empty err ' -test_expect_success 'run_command is restricted to PATH' ' + +test_lazy_prereq RUNS_COMMANDS_FROM_PWD ' + write_script runs-commands-from-pwd <<-\EOF && + true + EOF + runs-commands-from-pwd >/dev/null 2>&1 +' + +test_expect_success !RUNS_COMMANDS_FROM_PWD 'run_command is restricted to PATH' ' write_script should-not-run <<-\EOF && echo yikes EOF
Re: [PATCH] Do not fail test if '.' is part of $PATH
Jeff King writes: > Since the test is ultimately checking "can we run should-not-run from > the current directory", might it be simpler to actually try that as the > precondition? I.e., something like: > ... A nice egg of columbus. It also would save us from mischievous users who have should-not-run somewhere no the $PATH that outputs the string we expect (no, I do not think it is a common thing to do; I am just saying that the solution covers such an extremely stupid case without special casing).
Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files
Thomas Gummerer writes:
> Agreed, I think --{no-,}overlay is a much better name for the option,
> I'll use that for my patch series (I hope to send that soon after 2.20
> is released).
OK.
> I must admit that I was not aware that the mode is called overlay
> mode, before you explained it to me, so I wouldn't expect most users
> to know either. But as it's easy to explain that probably doesn't
> matter much.
I do not think "the mode is called the overlay mode" is so accurate
a description. I think I've seen the word 'overlay' used to
describe the behaviour in earlier discussions, but because there is
no 'non-overlay' mode exists in versions of 'git checkout' the
end-users have, the users won't even be aware of the possibility
that mode different from what they are used to see could exist, or
that the mode that they are used to see could be called/explained as
the 'overlay' mode. IOW, we should pick the best phrase to explain
the behaviour we can use when coming up with the command line
option, and that phrase does not have to be 'overlay'---there is no
"using the word 'overlay' for this is good because the users are
familiar with the existing use of the word", simply because there
isn't such familiarilty ;-)
Re: [L10N] Kickoff for Git 2.20.0 round 3
Jiang Xin writes: > Git v2.20.0-rc2 has been released, and there are 5 new messages need to > be translated. So let's start new round of l10n for Git 2.20.0. A huge thanks, as always, to the translation team. Jiang, sorry to see that -rc2 slipped just after you sent out the round 2 message and you needed to start round 3 just after it.
Re: [RFC] git clean --local
"Randall S. Becker" writes: > Would something like git clean --exclude=file-pattern work as a > compromise notion? Files matching the pattern would not be cleaned > regardless of .gitignore or their potential preciousness status > long-term. Multiple repetitions of the --exclude option might be > supportable. I could see that being somewhat useful in scripting. I think "git clean" already takes "-e", but I am not sure if it is meant to do what you wrote above. If "git clean" takes a pathspec, perhaps you can give a negative pathspec to exclude whatever you do not want to get cleaned, something like git clean '*.o' ':!precious.o' to say "presious.o is ignored (hence normally expendable), but I do not want to clean it with this invocation of 'git clean'"?
Re: [RFC] git clean --local
Junio C Hamano writes: > Cameron Boehmer writes: > >> 1) add a new flag >> -l, --local >> Do not consult git config --global core.excludesFile in >> determining what files git ignores. This is useful in conjunction with >> -x/-X to preserve user files while removing build artifacts. > ... > But it might be useful as an option that affects any "git" command, > e.g. "git --local-config-only clean". I dunno. If you only want to say "there is no global excludes file", perhaps $ git -c core.excludesFile=/dev/null clean -x may be sufficient, so for that particular use case, there is no need to introduce a new command, I'd think. In the longer term, however, I think we would want to introduce a distinction among ignored files---we only support "ignored and expendable" class, but not "ignored but precious" class. With the latter class introduced, it would make sense for "git clean -x/-X" to notice that a path is ignored but precious and keep it. If a dir/foo is ignored, dir/bar is tracked in commit A that is currently checked out, and there is no dir/ directory in commit B, checking out commit B would remove dir/foo (because the last tracked file in the directory goes away and all remaining files in the directory would be ignored but expendable). But if we introduced a new "ignored but precious" class and made dir/foo a member of such a class, then you will be prevented from checkout out B until you do something about dir/foo that is now "precious".
Re: [RFC] git clean --local
Cameron Boehmer writes: > 1) add a new flag > -l, --local > Do not consult git config --global core.excludesFile in > determining what files git ignores. This is useful in conjunction with > -x/-X to preserve user files while removing build artifacts. This does not belong to the "clean" command (who says the need to ignore the global configuration is limited to "clean" and why?), so "git clean --local" is simply not acceptable. But it might be useful as an option that affects any "git" command, e.g. "git --local-config-only clean". I dunno. > 2) change the behavior of -x/-X This won't happen without a long deprecation period. If you haven't seen and read them, check the recent list archive for the past few weeks, with keywords "trashable", "precious", etc.
Re: [PATCH] t6036: avoid "cp -a"
Elijah Newren writes:
> Thanks for the patch!
>
> On Fri, Nov 30, 2018 at 6:52 PM Carlo Marcelo Arenas Belón
> ...
> Oops. Thanks for catching. To be honest, we don't even need -a, -R,
> etc. -- it was just a habit for me to add -a after cp. A simple cp
> would do, though what you have here is fine too.
Thanks, both. I think the topic won't escape 'next' until 2.20
final, and after that we can rewind 'next', so it may be better
to squash it in, but anyway...
-- >8 --
From: Carlo Marcelo Arenas Belón
Date: Fri, 30 Nov 2018 18:52:12 -0800
Subject: [PATCH] t6036: avoid non-portable "cp -a"
b8cd1bb713 ("t6036, t6043: increase code coverage for file collision
handling", 2018-11-07) uses this GNU extension that is not available
in a POSIX complaint cp. In this particular case, there is no need to
use the option, as it is just copying a single file to create another
file.
Signed-off-by: Carlo Marcelo Arenas Belón
Signed-off-by: Junio C Hamano
---
t/t6036-recursive-corner-cases.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t6036-recursive-corner-cases.sh
b/t/t6036-recursive-corner-cases.sh
index b7488b00c0..d23b948f27 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -1631,7 +1631,7 @@ test_expect_success 'check nested conflicts' '
# Compare m to expected contents
>empty &&
- cp -a m_stage_2 expected_final_m &&
+ cp m_stage_2 expected_final_m &&
test_must_fail git merge-file --diff3 \
-L "HEAD" \
-L "merged common ancestors" \
--
2.20.0-rc1-10-g7068cbc4ab
Re: en/rebase-merge-on-sequencer, was Re: What's cooking in git.git (Nov 2018, #07; Fri, 30)
Johannes Schindelin writes: > Hi Junio, > > On Fri, 30 Nov 2018, Junio C Hamano wrote: > >> * en/rebase-merge-on-sequencer (2018-11-08) 2 commits >> - rebase: implement --merge via git-rebase--interactive >> - git-rebase, sequencer: extend --quiet option for the interactive machinery >> >> "git rebase --merge" as been reimplemented by reusing the internal >> machinery used for "git rebase -i". > > I *think* a new iteration has landed (which has 7 instead of 2 commits): > https://public-inbox.org/git/20181122044841.20993-1-new...@gmail.com/ "Landed" as opposed to "be in-flight"? You got me worried by implying that I merged them to either 'master' or 'next' where it is harder to back out ;-). During the freeze, especially after -rc1, I stop paying attention to anything other than regression fixes and fixes to the addition since the previous releases, unless I have too much time and get bored and the new topic is trivial (which often means a single patch). I'll mark the topic with the following, and continue ignoring it (or any other topics) for now. Thanks. * en/rebase-merge-on-sequencer (2018-11-08) 2 commits - rebase: implement --merge via git-rebase--interactive - git-rebase, sequencer: extend --quiet option for the interactive machinery "git rebase --merge" as been reimplemented by reusing the internal machinery used for "git rebase -i". Reroll exists. cf. <20181122044841.20993-1-new...@gmail.com>
What's cooking in git.git (Nov 2018, #07; Fri, 30)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. The road to the upcoming 2.20 turned out to be a bit rockier as we had a couple of subcommands with larger importance (rebase and rebase-i) reimplemented, together with some new and exciting commands (like range-diff and its integration into format-patch), each with a few loose ends we needed to tie until the last minute. I've let -rc1 and -rc2 slip for a few days to make sure we get closer to a stable point, and I am hoping that a few topics that are at the bottom of master..pu chain with today's pushout merged to the 'master' branch, I should be able to cut a 2.20-rc2 that is in a reasonable shape. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [New Topics] * gh/diff-raw-has-no-ellipses (2018-11-26) 1 commit (merged to 'next' on 2018-11-29 at 24a7536f15) + doc: update diff-format.txt for removed ellipses in --raw "git diff --raw" lost ellipses to adjust the output columns for some time now, but the documentation still showed them. Will cook in 'next'. * mk/http-backend-kill-children-before-exit (2018-11-26) 1 commit (merged to 'next' on 2018-11-29 at bf2d45062f) + http-backend: enable cleaning up forked upload/receive-pack on exit The http-backend CGI process did not correctly clean up the child processes it spawns to run upload-pack etc. when it dies itself, which has been corrected. Will cook in 'next'. * ab/replace-graft-with-replace-advice (2018-11-29) 1 commit (merged to 'next' on 2018-11-30 at c5d658e075) + advice: don't pointlessly suggest --convert-graft-file The advice message to tell the user to migrate an existing graft file to the replace system when a graft file was read was shown even when "git replace --convert-graft-file" command, which is the way the message suggests to use, was running, which made little sense. Will merge to 'master'. * ma/reset-doc-rendering-fix (2018-11-29) 2 commits (merged to 'next' on 2018-11-30 at be718c19e2) + git-reset.txt: render literal examples as monospace + git-reset.txt: render tables correctly under Asciidoctor Doc updates. Will merge to 'master'. * sg/daemon-test-signal-fix (2018-11-27) 1 commit (merged to 'next' on 2018-11-30 at b3f7fdf727) + t/lib-git-daemon: fix signal checking Test fix. Will merge to 'master'. * tb/log-G-binary (2018-11-29) 1 commit - log -G: ignore binary files "git log -G" looked for a hunk in the "git log -p" patch output that contained a string that matches the given pattern. Optimize this code to ignore binary files, which by default will not show any hunk that would match any pattern (unless textconv or the --text option is in effect, that is). Expecting an update to the tests. * jc/format-patch-range-diff-fix (2018-11-30) 1 commit (merged to 'next' on 2018-11-30 at 26290b1ec1) + format-patch: do not let its diff-options affect --range-diff "git format-patch --range-diff" by mistake passed the diff options used to generate the primary output of the command to the range-diff machinery, which caused the range-diff in the cover letter to include fairly useless "--stat" output. This has been corrected by forcing a non-customizable default formatting options on the range-diff machinery when driven by format-patch. Will merge to 'master'. * js/rebase-reflog-action-fix (2018-11-30) 1 commit (merged to 'next' on 2018-11-30 at 93fd2fb920) + rebase: fix GIT_REFLOG_ACTION regression "git rebase" reimplemented recently in C accidentally changed the way reflog entries are recorded (earlier "rebase -i" identified the entries it leaves with "rebase -i", but the new version always marks them with "rebase"). This has been corrected. Will merge to 'master'. * js/rebase-stat-unrelated-fix (2018-11-30) 1 commit (merged to 'next' on 2018-11-30 at a9faaff8c1) + rebase --stat: fix when rebasing to an unrelated history "git rebase --stat" to transplant a piece of history onto a totally unrelated history were not working before and silently showed wrong result. With the recent reimplementation in C, it started to instead die with an error message, as the original logic was not prepared to cope with this case. This has now been fixed. Will merge to 'master'. -- [Graduated to "master"] * cc/delta-islands (2018-11-21) 3 commits (merged to 'next' on 2018-11-21 at 3bac399f83) + pack-objects: fix off-by-one in delta-island tree-depth computation + pack-objects: zero-initialize tree_depth/layer arrays + pack-objects: fix tree_depth and layer invariants A few issues in the
Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)
Junio C Hamano writes: >> I had to delay -rc2 to see these last minute tweaks come to some >> reasonable place to stop at, and I do not think we want to delay the >> final any longer or destablizing it further by piling last minute >> undercooked changes on top. > > So how about doing this on top of 'master' instead? As this leaks > *no* information wrt how range-diff machinery should behave from the > format-patch side by not passing any diffopt, as long as the new > code I added to show_range_diff() comes up with a reasonable default > diffopts (for which I really would appreciate extra sets of eyes to > make sure), this change by definition cannot be wrong (famous last > words). As listed in today's "What's cooking" report, I've merged this to 'next' in today's pushout and planning to have it in the -rc2. I am not married to this exact implementation, and I'd welcome to have an even simpler and less disruptive solution if exists, but I am hoping that this is a good-enough interim measure for the upcoming release, until we decide what to do with the customizability of range-diff driven by format-patch. In addition to this, I am planning the "rebase --stat" and "reflog that does not say 'rebase -i' but 'rebase'" fixes merged to 'master' before cutting -rc2.
Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files
Duy Nguyen writes: > core.uiVersion is a big no no to me. I don't want to go to someone's > terminal, type something and have a total surprise because they set > different ui version. If you want a total UI redesign, go with a new > prefix, like "ng" (for new git) or something instead of "git". Yup, very good point to keep in mind.
Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files
Duy Nguyen writes: >> >> OK. Is "auto-vivify the named branch based on a remote-tracking" >> also rejected, as it is a confusing behaviour that is a too subtle >> and implicit, just like the detaching head is, and require --guess >> or sticking to 'git checkout'? I think it should. > > This touches the "remote" concept which I think is another confusing > thing for new people (your "master" is not the same as the server's > "master", aka origin/master) and perhaps this dwim thing helps. > Frankly I don't do dwim much so I don't know if it's that often used. I actually think a user who sees a DWIM without understanding what the user wants to do would perceive magic that sometimes works and sometimes does not, and some other times it does a random thing that the user does not even understand what is going on. And such a random magic that sometimes works, even if the "sometimes" is "most of the time", say 85% of the time, would not help user form the right mental model. "git checkout master~2" that DWIMs to "git checkout --deatch master~2", but does totally different thing when "git checkout master" is given, leaving the user confused "what is so different between these two?". Until the user realizes 'master' can serve both as a branch name and a name for a commit object, while master~2 can only be a name for a commit object and is not a branch name, the behaviour of the command will stay to be mysterious and DWIMmage would not help user form the right mental model. I earlier said that I agree with your decision to leave the implied form out of switch-branch for exactly that reason. The behaviour falls into the same category as "git checkout frotz" that DWIMS to "git checkout -b frotz -t remotes/origin/frotz", which also is mysterious until the user understands your 'master' is unique and is different from 'master' to everybody else, I would think.
[PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)
Junio C Hamano writes:
> In any case, I tend to agree with the conclusion in the downthread
> by Dscho that we should just clearly mark that invocations of the
> "format-patch --range-diff" command with additional diff options is
> an experimental feature that may not do anything sensible in the
> upcoming release, and declare that the UI to pass diff options to
> affect only the range-diff part may later be invented. IOW, I am
> coming a bit stronger than Dscho's suggestion in that we should not
> even pretend that we aimed to make the options used for range-diff
> customizable when driven from format-patch in the upcoming release,
> or aimed to make --range-diff option compatible with other diff
> options given to the format-patch command.
>
> I had to delay -rc2 to see these last minute tweaks come to some
> reasonable place to stop at, and I do not think we want to delay the
> final any longer or destablizing it further by piling last minute
> undercooked changes on top.
So how about doing this on top of 'master' instead? As this leaks
*no* information wrt how range-diff machinery should behave from the
format-patch side by not passing any diffopt, as long as the new
code I added to show_range_diff() comes up with a reasonable default
diffopts (for which I really would appreciate extra sets of eyes to
make sure), this change by definition cannot be wrong (famous last
words).
-- >8 --
Subject: format-patch: do not let its diff-options affect --range-diff
Stop leaking how the primary output of format-patch is customized to
the range-diff machinery and instead let the latter use its own
"reasonable default", in order to correct the breakage introduced by
a5170794 ("Merge branch 'ab/range-diff-no-patch'", 2018-11-18) on
the 'master' front. "git format-patch --range-diff..." without any
weird diff option started to include the "range-diff --stat" output,
which is rather useless right now, that made the whole thing
unusable and this is probably the least disruptive way to whip the
codebase into a shippable shape.
We may want to later make the range-diff driven by format-patch more
configurable, but that would have to wait until we have a good
design.
Signed-off-by: Junio C Hamano
---
Documentation/git-format-patch.txt | 5 +
builtin/log.c | 2 +-
log-tree.c | 2 +-
range-diff.c | 6 +-
range-diff.h | 5 +
5 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-format-patch.txt
b/Documentation/git-format-patch.txt
index aba4c5febe..27304428a1 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -250,6 +250,11 @@ feeding the result to `git send-email`.
feature/v2`), or a revision range if the two versions of the series are
disjoint (for example `git format-patch --cover-letter
--range-diff=feature/v1~3..feature/v1 -3 feature/v2`).
++
+Note that diff options passed to the command affect how the primary
+product of `format-patch` is generated, and they are not passed to
+the underlying `range-diff` machinery used to generate the cover-letter
+material (this may change in the future).
--creation-factor=::
Used with `--range-diff`, tweak the heuristic which matches up commits
diff --git a/builtin/log.c b/builtin/log.c
index 0fe6f9ba1e..5ac18e2848 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1096,7 +1096,7 @@ static void make_cover_letter(struct rev_info *rev, int
use_stdout,
if (rev->rdiff1) {
fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
show_range_diff(rev->rdiff1, rev->rdiff2,
- rev->creation_factor, 1, >diffopt);
+ rev->creation_factor, 1, NULL);
}
}
diff --git a/log-tree.c b/log-tree.c
index 7a83e99250..b243779a0b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -762,7 +762,7 @@ void show_log(struct rev_info *opt)
next_commentary_block(opt, NULL);
fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
show_range_diff(opt->rdiff1, opt->rdiff2,
- opt->creation_factor, 1, >diffopt);
+ opt->creation_factor, 1, NULL);
memcpy(_queued_diff, , sizeof(diff_queued_diff));
}
diff --git a/range-diff.c b/range-diff.c
index 767af8c5bb..8e52a85c19 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -460,7 +460,11 @@ int show_range_diff(const char *range1, const char *range2,
struct diff_options opts;
struct strbuf indent = STRBUF_INIT;
- memcpy(, diffopt, sizeof(opts));
+ if (diffopt)
+ memcpy(, diffopt, sizeof(opts));
Re: [PATCH 3/5] pack-objects: add --sparse option
Derrick Stolee writes: > You're right that having this hidden as an opt-in config variable > makes it hard to discover as a typical user. > > I would argue that we should actually make the config setting true by > default, and recommend that servers opt-out. Here are my reasons: > > 1. The vast majority of users are clients. > > 2. Client users are not likely to know about and tweak these settings. > > 3. Server users are more likely to keep an eye on the different knobs > they can tweak. > > 4. Servers should use the reachability bitmaps, which don't use this > logic anyway. > > While _eventually_ we should make this opt-out, we shouldn't do that > until it has cooked a while. I actually do not agree. If the knob gives enough benefit, the users will learn about it viva voce, and in a few more releases we'll hear "enough users complain that they have to turn it on, let's make it the default". If that does not happen, the knob does not deserve to be turned on in the first place. The same applies to many shiny new toys people are discussing recently on this list (e.g. precious vs expendable and non-overlay checkout are the ones that immediately come to my mind). Having said that, I won't be commenting on this shiny new toy before the final. I want to see more people help tying the loose ends and give it final varnish to the upcoming release, as it seems to have become rockier and larger release than we originally anticipated.
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
Ævar Arnfjörð Bjarmason writes: >> What prevents you from using `sq_dequote_to_argv()`? > > I mean not just nasty in terms of implementation, yeah we could do it, > but also a nasty UX for things like --word-diff-regex. I.e. instead of: > > --range-diff-word-diff-regex='[0-9"]' > > You need: > > --range-diff-opts="--word-diff-regex='[0-9\"]'" > > Now admittedly that in itself isn't very painful *in this case*, but in > terms of precedent I really dislike that option, i.e. git having some > mode where I need to work to escape input to pass to another command. In addition, sq_dequote are meant to be used on quoted string we internally produce; I do not think we want to promise that it is safe to use on a random string that comes from end users. In any case, I tend to agree with the conclusion in the downthread by Dscho that we should just clearly mark that invocations of the "format-patch --range-diff" command with additional diff options is an experimental feature that may not do anything sensible in the upcoming release, and declare that the UI to pass diff options to affect only the range-diff part may later be invented. IOW, I am coming a bit stronger than Dscho's suggestion in that we should not even pretend that we aimed to make the options used for range-diff customizable when driven from format-patch in the upcoming release, or aimed to make --range-diff option compatible with other diff options given to the format-patch command. I had to delay -rc2 to see these last minute tweaks come to some reasonable place to stop at, and I do not think we want to delay the final any longer or destablizing it further by piling last minute undercooked changes on top.
Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files
Nguyễn Thái Ngọc Duy writes: > 'git switch-branch' > > - implicit detaching is rejected. If you need to detach, you need to > give --detach. Or stick to 'git checkout'. OK. Is "auto-vivify the named branch based on a remote-tracking" also rejected, as it is a confusing behaviour that is a too subtle and implicit, just like the detaching head is, and require --guess or sticking to 'git checkout'? I think it should. > - -b/-B is renamed to -c/-C with long option names I did not expect that these two are the only options that would be out of place with the command name split, but presumably you looked at all options for both of the two new commands to see if they made sense in the new context? > 'git restore-files' > > - takes a ref from --from argument, not as a free ref. As a result, > '--' is no longer needed. All non-option arguments are pathspec OK. That does make things easier to teach, as there is no need for disambiguation. > - pathspec is mandatory, you can't do "git restore-files" without any > pathspec. > > - I just remember -p which is allowed to take no pathspec :( I'll fix > it later. Or leave it out of restore-files as a more advanced feature (just like detaching with HEAD^0 is left out of switch-branch) that the user can stick to 'git checkout' to use. > - Two more fancy features (the "git checkout --index" being the > default mode and the backup log for accidental overwrites) are of > course still missing. But they are coming. I am unsure about the wisdom of calling it "--index", though. The "--index" option is "the command can work only on the index, or only on the working tree files, or on both the index and the working tree files, and this option tells it to work in the 'both the index and the working tree files' mode", but when "restore-files" touches paths, it always modifies both the index and the working tree, so the "--index" option does not capture the differences well in this context [*1*]. As I saw this was described as "not using the usual 'overlay' semantics [*2*]", perhaps --overlay/--no-overlay option that defaults to --no-overlay is easier to explain. side note 1. I think the original mention of "--index" came in the context of contrasting "git reset" with "git checkout". "git reset (--hard|--mixed) -- " (that does not move HEAD), which does not but perhaps should exist, is very much like "git checkout -- ", and if "reset" were written after the "--index/--cached" convention was established, "reset --hard" would have called "reset --index" while "reset --mixed" would have been "reset --cached" (i.e. only work on the index and not on the working tree). And "reset --index " would have worked by removing paths in that are not in the HEAD and updating paths in that are in the HEAD, i.e. identical to the non overlay behaviour proposed for the "git checkout" command. So calling the non overlay mode "--index" makes sense in the context of discussing "git reset", and not in the context of "git checkout". side note 2. "git checkout " grabs entries from the that patch and adds them to the index and checks them out to the working tree. If the original index has entries that match but do not appear in , they are left in the result. That is "overlaying what was taken from the on top of what is in the index". Having said all that, I will not be looking at the series until 2.20 final. And I hope more people do the same to concentrate on helping us prevent the last minute glitch slipping in the final release. Thanks.
Re: [PATCH/RFC v2 0/7] Introduce new commands switch-branch and checkout-files
Duy Nguyen writes: > On Wed, Nov 28, 2018 at 9:01 PM Duy Nguyen wrote: >> should we do >> something about detached HEAD in this switch-branch command (or >> whatever its name will be)? >> >> This is usually a confusing concept to new users > > And it just occurred to me that perhaps we should call this "unnamed > branch" (at least at high UI level) instead of detached HEAD. It is > technically not as accurate, but much better to understand. As I said elsewhere in nearby thread, I agree that "unnamed branch" is a reasonable way to explain what the state the user is in. It is not incorrect per-se that HEAD is detached from anything in refs/ in such a state, but that is an implementation detail of how the worktree gets on the unnamed branch (which lasts until the worktree next gets on a named branch, at which point the unnamed branch disappears).
Re: [PATCH 1/1] rebase --stat: fix when rebasing to an unrelated history
Johannes Schindelin writes: > But I guess that I should not be so lazy and really use two different > messages here: > > Changes from to > > and if there is no merge base, > > Changes in Ah, that's excellent. Thanks.
Re: [PATCH] pack-protocol.txt: accept error packets in any context
Masaya Suzuki writes: > Yes, I did. And it also didn't end up in a build error. Do I have a > different build option...? Passig DEVELOPER=Yes to make turns a bit more warnings on (in this case, I think it was "unused-variable") and also uses -Werror to turn warnings into errors.
Re: [PATCH] pack-protocol.txt: accept error packets in any context
Masaya Suzuki writes: > In the Git pack protocol definition, an error packet may appear only in > a certain context. However, servers can face a runtime error (e.g. I/O > error) at an arbitrary timing. This patch changes the protocol to allow > an error packet to be sent instead of any packet. > > Following this protocol spec change, the error packet handling code is > moved to pkt-line.c. > > Signed-off-by: Masaya Suzuki > --- Have you run tests with this applied? I noticed 5703.9 now has stale expectations, but there may be others.
Re: [PATCH] pack-protocol.txt: accept error packets in any context
Junio C Hamano writes:
>> diff --git a/connect.c b/connect.c
>> index 24281b608..458906e60 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -306,8 +306,6 @@ struct ref **get_remote_heads(struct packet_reader
>> *reader,
>> die_initial_contact(1);
>> case PACKET_READ_NORMAL:
>> len = reader->pktlen;
>> -if (len > 4 && skip_prefix(reader->line, "ERR ", ))
>> -die(_("remote error: %s"), arg);
>> break;
>> case PACKET_READ_FLUSH:
>> state = EXPECTING_DONE;
This breaks build by not removing the decl of arg while removing the
last user of that variable in the function.
connect.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/connect.c b/connect.c
index 458906e60d..4813f005ab 100644
--- a/connect.c
+++ b/connect.c
@@ -296,7 +296,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,
struct ref **orig_list = list;
int len = 0;
enum get_remote_heads_state state = EXPECTING_FIRST_REF;
- const char *arg;
*list = NULL;
--
2.20.0-rc1-10-g7068cbc4ab
Re: [PATCH v2] log -G: Ignore binary files
Junio C Hamano writes: >> +test_expect_success 'log -G ignores binary files' ' >> +git checkout --orphan orphan1 && >> +printf "a\0a" >data.bin && >> +git add data.bin && >> +git commit -m "message" && >> +git log -Ga >result && >> +test_must_be_empty result >> +' > > As this is the first mention of data.bin, this is adding a new file > data.bin that has two 'a' but is a binary file. And that is the > only commit in the history leading to orphan1. > > The fact that "log -Ga" won't find any means it missed the creation > event, because the blob is binary. Good. By the way, this root commit records another file whose path is "file" and has "Picked" in it. If the file had 'a' in it, it would have been included in "git log" output, but that is too subtle a point to be noticed by the readers who are only reading this patch without seeing what has been done to the index before this test piece. If you are going to restructure these tests to create a three-commit history in a single expect_success that is inspected with various "log -Ga" invocations in subsequent tests, it is worth removing that other file (or rather, starting with "read-tree --empty" immediately after checking out the orphan branch, to clarify to the readers that there is nothing but what you add in the set-up step in the index) to make the test more robust.
Re: [PATCH v2] log -G: Ignore binary files
Thomas Braun writes: > Subject: Re: [PATCH v2] log -G: Ignore binary files s/Ig/ig/; (will locally munge--this alone is no reason to reroll). The code changes looked sensible. > diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh > index 844df760f7..5c3e2a16b2 100755 > --- a/t/t4209-log-pickaxe.sh > +++ b/t/t4209-log-pickaxe.sh > @@ -106,4 +106,44 @@ test_expect_success 'log -S --no-textconv (missing > textconv tool)' ' > rm .gitattributes > ' > > +test_expect_success 'log -G ignores binary files' ' > + git checkout --orphan orphan1 && > + printf "a\0a" >data.bin && > + git add data.bin && > + git commit -m "message" && > + git log -Ga >result && > + test_must_be_empty result > +' As this is the first mention of data.bin, this is adding a new file data.bin that has two 'a' but is a binary file. And that is the only commit in the history leading to orphan1. The fact that "log -Ga" won't find any means it missed the creation event, because the blob is binary. Good. > +test_expect_success 'log -G looks into binary files with -a' ' > + git checkout --orphan orphan2 && > + printf "a\0a" >data.bin && > + git add data.bin && > + git commit -m "message" && This starts from the state left by the previous test piece, i.e. we have a binary data.bin file with two 'a' in it. We pretend to modify and add, but these two steps are no-op if the previous succeeded, but even if the previous step failed, we get what we want in the data.bin file. And then we make an initial commit the same way. > + git log -a -Ga >actual && > + git log >expected && And we ran the same test but this time with "-a" to tell Git that binary-ness should not matter. It will find the sole commit. Good. > + test_cmp actual expected > +' > + > +test_expect_success 'log -G looks into binary files with textconv filter' ' > + git checkout --orphan orphan3 && > + echo "* diff=bin" > .gitattributes && s/> />/; (will locally munge--this alone is no reason to reroll). > + printf "a\0a" >data.bin && > + git add data.bin && > + git commit -m "message" && > + git -c diff.bin.textconv=cat log -Ga >actual && This exposes a slight iffy-ness in the design. The textconv filter used here does not strip the "binary-ness" from the payload, but it is enough to tell the machinery that -G should look into the difference. Is that really desirable, though? IOW, if this weren't the initial commit (which is handled by the codepath to special-case creation and deletion in diff_grep() function), would "log -Ga" show it without "-a"? Should it? I think this test piece (and probably the previous ones for "-a" vs "no -a" without textconv, as well) should be using a history with three commits, where - the root commit introduces "a\0a" to data.bin (creation event) - the second commit adds another instance of "a\0a" to data.bin (forces comparison) - the third commit removes data.bin (deletion event) and make sure that the three are treated identically. If "log -Ga" finds one (with the combination of other conditions like use of textconv or -a option), it should find all three, and vice versa. > + git log >expected && > + test_cmp actual expected > +' > + > +test_expect_success 'log -S looks into binary files' ' > + git checkout --orphan orphan4 && > + printf "a\0a" >data.bin && > + git add data.bin && > + git commit -m "message" && > + git log -Sa >actual && > + git log >expected && > + test_cmp actual expected > +' Likewise. This would also benefit from a three-commit history. Perhaps you can create such a history at the beginning of these additions as another "setup -G/-S binary test" step and test different variations in subsequent tests without the setup? > test_done
Re: [PATCH 5/5] test-lib: add support for GIT_TODO_TESTS
Ævar Arnfjörð Bjarmason writes: > -To skip tests, set the GIT_SKIP_TESTS variable. Individual tests can > -be skipped: > +To skip tests, set either the GIT_SKIP_TESTS or GIT_TODO_TESTS > +variables. The difference is that with SKIP the tests won't be run at > +all, whereas they will be run with TODO, but in success or failure > +annotated as a TODO test. It is entirely unclear what "They will be run with TODO" means. I know what you want to achieve from the code change; instead of just skipping, you want to cause as much side effect the skipped test piece wants to make until it fails and dies, without taking the remainder of the test down. And I kind of agree that sometimes such a mode would be very useful (i.e. when you do not want to bother separating such a failing test properly into the setup part that would affect later tests and the one-thing-it-wants-to-test part that can now be safely skipped). From the cursory look, the code change in this patch is sensible realization of that idea. Having said all that, I won't picking this up until next month ;-) I really want to see that everybody is concentrating on making sure that 2.20 is solid before playing with shiny new toys. Thanks.
Re: [PATCH] transport-helper.c: do not translate a string twice
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> My bad.
>
> transport-helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 7213fa0d32..bf225c698f 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -573,7 +573,7 @@ static int run_connect(struct transport *transport,
> struct strbuf *cmdbuf)
> fprintf(stderr, "Debug: Falling back to dumb "
> "transport.\n");
> } else {
> - die(_(_("unknown response to connect: %s")),
> + die(_("unknown response to connect: %s"),
> cmdbuf->buf);
Thanks.
Re: [PATCH v2 5/7] checkout: split options[] array in three pieces
Nguyễn Thái Ngọc Duy writes:
> +static struct option *add_switch_branch_options(struct checkout_opts *opts,
> + struct option *prevopts)
> +{
> + struct option options[] = {
> OPT_STRING('b', NULL, >new_branch, N_("branch"),
> N_("create and checkout a new branch")),
I think there should be another step to rename the options to more
sensible ones for the context. In the context of overall "checkout"
command, the 'b' option
git checkout -b "
that signals that its parameter has something to do with a 'branch'
makes perfect sense. But when the user uses the new command
git switch-branch -b
does not convey the more important fact in the context. In the
orignal context, "this is about a branch" and "we are not checking
out an existing one, but are creating" are both worthwhile thing to
express, but because a single letter option cannot stand for both,
"-b" is the most reasonable choice (compared to calling it "-c" that
stands for "create" that invites "what exactly are you creating?").
In the new context of "switch-branch", it no longer has to be said
that the optional feature is about "branch". So I would imagine
that users naturally expect this option to be called
git switch-branch --create
(or -c for short).
I'll just stop with this single example, but I think we should make
sure all the options make sense in the context of new command.
Of course, that means it will NOT be sufficient to just split the
option table into two tables and stitch them together for the single
command. This option must stay to be "-b" (giving it a synonym
"--create-branch" is OK) in the context of "checkout".
Re: [PATCH/RFC v2 0/7] Introduce new commands switch-branch and checkout-files
Stefan Xenos writes:
> So - IMO - detaching should always be an explicit action. Some options
> that occur to me:
>
> git switch-branch --detach
That is the most obvious way to spell it, and it is why we have "git
checkout --detach". If we were to split one half of "checkout" into
"switch-branch", I would support the idea to make switch-branch only
take a branch name and nothing else, allow it to take any commit-ish
to detach the HEAD at that commit in the history graph with the
--detach option". I also do not think anybody minds explaining the
resulting state to be "on an unnamed branch" (or is it "the" unnamed
branch? Given that HEAD@{} reflog is a singleton, perhaps the right
mental model is that there is only one unnamed branch per worktree).
> git detach
The detached HEAD state is not all that special to deserve a
separate command. After all, all history growing commands like
"commit", "cherry-pick", "rebase", "merge", etc. work the same way
on the unnamed branch.
> git switch-branch HEAD
I do not think this one is acceptable. "git checkout HEAD" does not
detach but works as if you said "git checkout master" (when on the
'master' branch). And you do not want "git switch-branch HEAD^0" to
be that explicit way to tell Git to detach the HEAD as that will
take us back to square one ("git checkout HEAD^0" is the more
concise and time-honored way to say "git checkout --detach HEAD").
Re: [PATCH v2 7/7] Suggest other commands instead of "git checkout"
Duy Nguyen writes: > I see my deliberate attempt to provoke has failed :D Giving your view > of the new commands as "training wheels", I take it we still should > make them visible as much as possible, but we just not try to hide > "git checkout" as much (e.g. we mention both new and old commands, > instead of just mentioning the new one, when suggesting something)? Yes, I do support the overall idea of learning two (or possibly three) separate commands would help new users to form the right mental model much sooner than learning one that can be used in multiple ways. Another possible approach could be to split the use of "reset" that does not move "HEAD" into the same half of "checkout" that does not move "HEAD", i.e. checkout-files.
Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files
Stefan Xenos writes: > Although I have no problem with "switch-branch" as a command name, > some alternative names we might consider for switch-branch might be: > > chbranch > swbranch Please never go in that direction. So far, we made a conscious effort to keep the names of most frequently used subcommand to proper words that can be understood by coders (IOW, I expect they know what 'grep' is, even though that may not be a 'proper word'). > switch > branch change (as a subcommand for the "branch" command) It is more like moving to the branch to work on. I think 'switch' is something SVN veterans may find familiar. Both are much better than ch/swbranch that are overly cryptic.
Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files
Stefan Beller writes: > I dislike the checkout-* names, as we already have checkout-index > as plumbing, so it would be confusing as to which checkout-* command > should be used when and why as it seems the co-index moves > content *from index* to the working tree, but the co-files moves content > *to files*, whereas checkout-branch is neither 'moving' to or from a branch > but rather 'switching' to that branch. To me, "switching to work on the branch", is like "checking the book out from the library to read". IOW, "check the branch out to work on" does not have to involve *any* moving of contents.
Re: [PATCH 1/1] rebase --stat: fix when rebasing to an unrelated history
"Johannes Schindelin via GitGitGadget"
writes:
> The built-in version of the `git rebase` command blindly translated that
> shell script code, assuming that there is no need to test whether there
> *was* a merge base, and due to its better error checking, exited with a
> fatal error (because it tried to read the object with hash ...
> as a tree).
And the scripted version produced a wrong result because it did not
check the lack of merge base and fed an empty string (presumably, as
that is what you would get from mb=$(merge-base A B) when A and B
are unrelated) to later steps? If that is the case, then it *is* a
very significant thing to record here. As it was not merely "failed
to stop due to lack of error checking", but a lot worse. It was
producing a wrong result.
A more faithful reimplementation would not have exited with fatal
error, but would have produced the same wrong result, so "a better
error checking caused the reimplementation die where the original
did not die" may be correct but is much less important than the fact
that "the logic used in the original to produce diffstat forgot that
there may not be a merge base and would not have worked correctly in
such a case, and that is what we are correcting" is more important.
Please descibe the issue as such, if that is the case (I cannot read
from the description in the proposed log message if that is the
case---but I came to the conclusion that it is what you wanted to
fix reading the code).
> if (options.flags & REBASE_VERBOSE)
> printf(_("Changes from %s to %s:\n"),
> - oid_to_hex(_base),
> + is_null_oid(_base) ?
> + "(empty)" : oid_to_hex(_base),
I am not sure (empty) is a good idea for two reasons. It is going
to be inserted into an translated string without translation. Also
it is too similar to the fallback string used by some printf
implementations when NULL was given to %s by mistake.
If there weren't i18n issues, I would suggest to use "empty merge
base" or "empty tree" instead, but the old one would have shown
0{40}, so perhaps empty_tree_oid_hex() is a good substitute?
> @@ -1494,8 +1495,9 @@ int cmd_rebase(int argc, const char **argv, const char
> *prefix)
> DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> opts.detect_rename = DIFF_DETECT_RENAME;
> diff_setup_done();
> - diff_tree_oid(_base, >object.oid,
> - "", );
> + diff_tree_oid(is_null_oid(_base) ?
> + the_hash_algo->empty_tree : _base,
> + >object.oid, "", );
The original would have run "git diff '' $onto", and died with an
error message, so both the original and the reimplementation were
failing. Just in different ways ;-)
> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> index b97ffdc9dd..be3b241676 100755
> --- a/git-legacy-rebase.sh
> +++ b/git-legacy-rebase.sh
> @@ -718,10 +718,12 @@ if test -n "$diffstat"
> then
> if test -n "$verbose"
> then
> - echo "$(eval_gettext "Changes from \$mb to \$onto:")"
> + mb_display="${mb:-(empty)}"
> + echo "$(eval_gettext "Changes from \$mb_display to \$onto:")"
> fi
> + mb_tree="${mb:-$(git hash-object -t tree /dev/null)}"
> # We want color (if set), but no pager
> - GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
> + GIT_PAGER='' git diff --stat --summary "$mb_tree" "$onto"
Code fix for diff-tree invocation, both in the builtin/ version and
this one, looks correct.
Re: [PATCH] config.mak.dev: enable -Wpedantic in clang
Eric Sunshine writes: > Playing Devi's Advocate, what if Apple's clang "8" was, in reality, > real-world clang 3? Then this condition would incorrectly enable the > compiler option on Apple for a (real) clang version below 4. For this > reason, it seems we shouldn't be trusting only the clang version > number when dealing with Apple. > > (I suspect that it won't make a difference in actual practice, so it > may be reasonable to punt on this issue until/if someone complains.) Why do we care which true version of clang is being used here in the first place? Is it because some version of clang (take -Wpedantic but misbehave | barf when -Wpedantic is given) while some others are OK? If the only problem is that some version of clang barf when the option is given, perhaps we can do a trial-compile of helloworld.c or something, or is that something we are trying to avoid in the first place? It appears to me that ./detect-compiler tool (by the way, perhaps we should start moving these build-helper-tools sitting at the top level down to ./build-helpers/ subdirectory or something so that we can focus on the source code when running "ls" at the top level of the hierarchy) can become more intelligent to help things like this.
Re: [RFC PATCH] Introduce "precious" file concept
Ævar Arnfjörð Bjarmason writes: > I don't think something like the endgame you've described in > https://public-inbox.org/git/xmqqzhtwuhpc@gitster-ct.c.googlers.com/ > is ever going to work. Novice git users (the vast majority) are not > going to diligently update both .gitignore and some .gitattribute > mechanism in lockstep. That goes both ways, no? Forcing people to add the same pattern, e.g. *.o, to .gitignore and .gitattribute to say they are expendable, when most of the time they are, is not something you should expect from the normal population. >> I would think that a proper automation needs per-path hint from the >> user and/or the project, not just a single-size-fits-all --force >> option, and "unlike all the *.o ignored files that are expendable, >> this vendor-supplied-object.o is not" is one way to give such a >> per-path hint. >> >>> This would give scripts which relied on our stable plumbing consistent >>> behavior, while helping users who're using our main porcelain not to >>> lose data. I could then add a --force option to the likes of read-tree >>> (on by default), so you could get porcelain-like behavior with >>> --no-force. >> >> At that low level, I suspect that a single size fits all "--force" >> would work even less well. > > Yeah I don't think the one-size-fits-all way out of this is a single > --force flag. Yes, indeed. That's why I prefer the "precious" bit. The system would behave the same way with or without it, but projects (not individual endusers) can take advantage of the feature if they wanted to.
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
Ævar Arnfjörð Bjarmason writes: > Since I raised this 'should we hold off?' I thought I'd chime in and say > that I'm fine with going along with what you suggest and having the > builtin as the default in the final. IOW not merge > jc/postpone-rebase-in-c down. OK.
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
Ævar Arnfjörð Bjarmason writes:
> + [--range-diff]]
Let's make sure a random string thrown at this mechanism will
properly get noticed and diagnosed.
> @@ -257,6 +258,13 @@ feeding the result to `git send-email`.
> creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
> for details.
>
> +--range-diff::
> + Other options prefixed with `--range-diff` are stripped of
> + that prefix and passed as-is to the diff machinery used to
> + generate the range-diff, e.g. `--range-diff-U0` and
> + `--range-diff--no-color`. This allows for adjusting the format
> + of the range-diff independently from the patch itself.
Taking anything is of course the most general, but I am afraid if
this backfires if there are some options that do not make sense to
be different between the invocations of range-diff and format-patch.
> @@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv,
> const char *prefix)
> rev.preserve_subject = keep_subject;
>
> argc = setup_revisions(argc, argv, , _r_opt);
> - if (argc > 1)
> - die(_("unrecognized argument: %s"), argv[1]);
> + if (argc > 1) {
> + struct argv_array args = ARGV_ARRAY_INIT;
> + const char *prefix = "--range-diff";
Please call that anything but "prefix" that hides the parameter to
the function.
const char *range_diff_opt = "--range-diff";
might work OK, or it might not. Let's read on.
> + int have_prefix = 0;
> +
> + for (i = 0; i < argc; i++) {
> + struct strbuf sb = STRBUF_INIT;
> + char *str;
> +
> + strbuf_addstr(, argv[i]);
> + if (starts_with(argv[i], prefix)) {
> + have_prefix = 1;
> + strbuf_remove(, 0, strlen(prefix));
> + }
> + str = strbuf_detach(, NULL);
> + strbuf_release();
> +
> + argv_array_push(, str);
> + }
> +
Is the body of the loop essentially this?
char *passopt = argv[i];
if (!skip_prefix(passopt, range_diff_opt, ))
saw_range_diff_opt = 1;
argv_array_push(, xstrdup(passopt));
We only use that "prefix" thing once, so we may not even need the
variable.
> + if (!have_prefix)
> + die(_("unrecognized argument: %s"), argv[1]);
So we take normal options and check the leftover args; if there is
no --range-diff among the leftover bits, we pretend that
we stumbled while reading the first such leftover arg.
> + argc = setup_revisions(args.argc, args.argv, _rev, NULL);
> + if (argc > 1)
> + die(_("unrecognized argument: %s"), argv[1]);
> + }
Otherwise, we pass all the leftover bits, which is a random mixture
but guaranteed to have at least one meant for range-diff, to another
setup_revisions(). If it leaves a leftover arg, then that is
diagnosed here, so we'd be OK (iow, this is not a new "attack
vector" to inject random string to command line parser).
One minor glitch I can see is "format-patch --range-diffSilly" would
report "unrecognised arg: Silly". As we are pretending to be and
reporting errors as format-patch, it would be better if we report
that --range-diffSilly was what we did not understand.
> Junio: I know it's late, but unless Eric has objections to this UI
> change I'd really like to have this in 2.20 since this is a change to
> a new command-line UI that's newly added in 2.20.
Quite honestly, I'd rather document "driving range-diff from
format-patch is experimental and does silly things when given
non-standard options in this release" and not touch the code at this
late stage in the game. Would it be less intrusive a change to
*not* support the --range-diff option, still use rd_rev
that is separate from the main rev, and use a reasonable hardcoded
default settings when preparing rd_rev?
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Johannes Sixt writes: > Am 27.11.18 um 00:31 schrieb Junio C Hamano: >> Johannes Sixt writes: >>> Am 26.11.18 um 04:04 schrieb Junio C Hamano: >>> ... this goes too far, IMO. It is the pager's task to decode control >>> characters. >> >> It was tongue-in-cheek suggestion to split a CR into caret-em on our >> end, but we'd get essentially the same visual effect if we added a >> rule: >> >> When producing a colored output (not limited to whitespace >> error coloring of diff output), insert before a CR >> that comes immediately before a LF. This was misspoken a bit. Let's revise it to When producing a colored output (not limited to whitespace error coloring of diff output) for contents that are not marked as eol=crlf (and other historical means), insert before a CR that comes immediately before a LF. to exempt CR in CRLF sequence that the contents and the user agree to be part of the end-of-line marker. > I wouldn't want that to happen for all output (context lines, - lines, > + lines): I really am not interested to see all the CRs in my CRLF > files. So your CRLF files will not give caret-em. >> And a good thing is that I do not think that new rule is doing any >> decode of control chars on our end. We are just producing colored >> output normally. > > But we already have it, as Brian pointed out: > >git diff --ws-error-highlight=old,new > > or by setting diff.wsErrorHighlight accordingly. Not really. If the context lines end with CRLF and the material is not CRLF, I do not think CR at the end of them should be highlighted as whitespace errors (as we tell to detect errors on "old" and "new" lines), but I think we still should make an effort to help them be visible to the users. Otherwise, next Frank will come and complain after seeing -something some common thing +something_new^M With the suggested change, you can distinguish the following two (and use your imagination that there are many "some common thing" lines), which would help the user guide if the file should be marked as CRLF file, or the contents mistakenly has some lines that end with CRLF by mistake. The corrective action between the two cases would vastly be different. -something^M-something some common thing^M some common thing some common thing^M some common thing some common thing^M some common thing +something_new^M+something_new^M Without, both would look like the RHS of the illustration, and with the "highlight both", you'd only get an extra caret-em on the removed line, and need to judge without the help from common context lines. Besides, --ws-error-highlight shows all whitespace errors, and showing CR as caret-em is mere side effect of the interaction between its coloring and the pager.
Re: in 2.19.2 t0061-run-command FAILs if . is in $PATH
Johannes Schindelin writes: > -test_expect_success 'run_command is restricted to PATH' ' > +test_lazy_prereq DOT_IN_PATH ' > + case ":$PATH:" in > + *:.:*) true;; > + *) false;; > + esac > +' An empty element in the colon-separated list also serves as an instruction to pick up executable from $cwd, so case ":$PATH:" in *:.:** | *::*) true ;; *) false ;; esac perhaps. > +test_expect_success !DOT_IN_PATH 'run_command is restricted to PATH' ' > write_script should-not-run <<-\EOF && > echo yikes > EOF > -- snap -- > > If so, can you please provide a commit message for it (you can add my > Signed-off-by: line and your Tested-by: line). > > Thanks, > Johannes
Re: Forcing GC to always fail
Bryan Turner writes: > For us, the biggest issue was "git gc"'s insistence on trying to run > "git reflog expire". That triggers locking behaviors that resulted in > very frequent GC failures--and the only reflogs Bitbucket Server (by > default) creates are all configured to never ex[ire or be pruned, so > the effort is all wasted anyway. Detecting that the expiry threshold is set to "never" before spending cycles and seeks to sift between old and new and not spawning the expire command? This seems like an obvious low-hanging fruit to me. > Another issue with the canned steps for "git gc" is that it means it > can't be used to do specific types of cleanup on a different schedule > from others. For example, we use "git pack-refs" directly to > frequently pack the refs in our repositories, separate from "git > repack" + "git prune" for repacking objects. That allows us to keep > our refs packed better without incurring the full overhead of > constantly building new packs. I am not sure if the above is an example of things that are good. We keep individual "pack-refs" and "rev-list | pack-objects" available exactly to give finer grained control to repository owners, and "gc" is meant to be one-size-fits-all easy to run by end users. Adding options to "git gc --no-reflog --pack-refs" to complicate it sounds somewhat backwards.
Re: [PATCH] advice: don't pointlessly suggest --convert-graft-file
Ævar Arnfjörð Bjarmason writes:
> The advice to run 'git replace --convert-graft-file' added in
> f9f99b3f7d ("Deprecate support for .git/info/grafts", 2018-04-29)
> didn't add an exception for the 'git replace --convert-graft-file'
> codepath itself.
>
> As a result we'd suggest running --convert-graft-file while the user
> was running --convert-graft-file, which makes no sense. Before:
>
> $ git replace --convert-graft-file
> hint: Support for /info/grafts is deprecated
> hint: and will be removed in a future Git version.
> hint:
> hint: Please use "git replace --convert-graft-file"
> hint: to convert the grafts into replace refs.
> hint:
> hint: Turn this message off by running
> hint: "git config advice.graftFileDeprecated false"
That's a good one. The glitch is real, the improvement is obvious,
and the implementation seems to be straight-forward and sensible.
How did you find one, is the real question ;-)
Re: Git pull confusing output
Will writes: > I’m far from being a guru, but I consider myself a competent Git > user. Yet, here’s my understanding of the output of one the most-used > commands, `git push`: >> Counting objects: 6, done. > No idea what an “object” is. Apparently there’s 6 of them > here. What does “counting” them means? Should I care? You vaguely recall that the last time you pushed you saw ~400 objects counted there, so you get the feeling how active you were since then. It is up to you if you are interested in such a feel of the level of activity. "git fetch" (hence "git pull") would also give you a similar "feel", e.g. "the last fetch was ~1200 objects and today's is mere ~200, so it seems it is a relatively slow day". As to "what is an object?", there are plenty of Git tutorials and books to learn the basics from. Again, it is up to you if you care. >> Delta compression using up to 4 threads. > No idea what is “delta compression”, I suppose something is being > compressed. It’s using anything between 1 and 4 threads, which is not > a very precise or useful information. Should I care? Likewise. >> Compressing objects: 100% (6/6), done. > I still don’t know what objects are, but I appreciate having feedback > on progress Exactly. >> Writing objects: 100% (6/6), 656 bytes | 656.00 KiB/s, done. > Writing what, where? Should I care? Still good to have feedback You are pushing the data in commits you wrote, modifications you made to files, etc., to the other side, so that is what is written to the other side. Is there any other thing you might suspect that is written in this context, to make you think a clarification is needed in the above message? >> Total 6 (delta 4), reused 0 (delta 0) > No idea what any of those numbers mean. Should I care? It is up to you to get interested in these details and learn what they mean. In this case, among these 6 objects transferred, Git managed to find that 4 are similar to other objects the other side already has or being sent by this push and can be transferred very efficiently by sending only the difference, which is what "delta" means. >> remote: Resolving deltas: 100% (4/4), completed with 4 local objects. > I do know what’s a remote, but I don’t know what “resolving > deltas” means. There’s local objects now? I don’t understand what > happened to those local objects, are they the byproduct of the delta > resolving or the input or something else? Should I care? The "remote:" prefix is "the other side said the following". IOW, you are seeing the message from the receiving end. As you sent 4 objects as mere "difference" (not the whole data needed to know every byte of the file or directory), the receiving side needed to find the object the "difference" was relative to, and reassemble what you would have sent if there weren't delta compression. These 4 local objects were local from the point of view of the other side, i.e. the repository that received your push. The information density of this one is much lower than the previous progress output lines. This one is primarily to give you the feeling of relative speed (you've seen how fast the "writing" phase which is constrained mostly by over-the-wire speed already, and now you are observing how many more seconds are spent to post-process the data sent over the wire) and avoiding to get you bored. I think we have "--quiet" option for those who do not care.
Re: [PATCH v2 7/7] Suggest other commands instead of "git checkout"
Nguyễn Thái Ngọc Duy writes:
> The assumption made is here
>
> - "git checkout" is a horrible monster that should only be touched
> with a two-meter pole
>
> - there are other commands that can achieve the same thing
Thanks for clearly spelling out the assumptions. It is good that
this step cames at the end, as the earlier 6 steps looked reasonable
to me.
Thanks.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> Documentation/git-branch.txt | 8 ++--
> Documentation/git-check-ref-format.txt | 2 +-
> Documentation/git-format-patch.txt | 2 +-
> Documentation/git-merge-base.txt | 2 +-
> Documentation/git-rebase.txt | 2 +-
> Documentation/git-remote.txt | 2 +-
> Documentation/git-rerere.txt | 10 ++---
> Documentation/git-reset.txt| 18 -
> Documentation/git-revert.txt | 2 +-
> Documentation/git-stash.txt| 6 +--
> Documentation/gitattributes.txt| 2 +-
> Documentation/gitcli.txt | 4 +-
> Documentation/gitcore-tutorial.txt | 18 -
> Documentation/giteveryday.txt | 24 ++--
> Documentation/githooks.txt | 5 ++-
> Documentation/gittutorial-2.txt| 2 +-
> Documentation/gittutorial.txt | 4 +-
> Documentation/revisions.txt| 2 +-
> Documentation/user-manual.txt | 54 +-
> advice.c | 2 +-
> sha1-name.c| 2 +-
> wt-status.c| 2 +-
> 22 files changed, 88 insertions(+), 87 deletions(-)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index bf5316ffa9..1564df47d2 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -48,7 +48,7 @@ The command's second form creates a new branch head named
>
> which points to the current `HEAD`, or if given.
>
> Note that this will create the new branch, but it will not switch the
> -working tree to it; use "git checkout " to switch to the
> +working tree to it; use "git switch-branch " to switch to the
> new branch.
>
> When a local branch is started off a remote-tracking branch, Git sets up the
> @@ -194,7 +194,7 @@ This option is only applicable in non-verbose mode.
> +
> This behavior is the default when the start point is a remote-tracking
> branch.
> Set the branch.autoSetupMerge configuration variable to `false` if you
> -want `git checkout` and `git branch` to always behave as if `--no-track`
> +want `git switch-branch` and `git branch` to always behave as if `--no-track`
> were given. Set it to `always` if you want this behavior when the
> start-point is either a local or remote-tracking branch.
>
> @@ -293,7 +293,7 @@ Start development from a known tag::
> $ git clone git://git.kernel.org/pub/scm/.../linux-2.6 my2.6
> $ cd my2.6
> $ git branch my2.6.14 v2.6.14 <1>
> -$ git checkout my2.6.14
> +$ git switch-branch my2.6.14
>
> +
> <1> This step and the next one could be combined into a single step with
> @@ -319,7 +319,7 @@ NOTES
> -
>
> If you are creating a branch that you want to checkout immediately, it is
> -easier to use the git checkout command with its `-b` option to create
> +easier to use the "git switch-branch" command with its `-b` option to create
> a branch and check it out with a single command.
>
> The options `--contains`, `--no-contains`, `--merged` and `--no-merged`
> diff --git a/Documentation/git-check-ref-format.txt
> b/Documentation/git-check-ref-format.txt
> index d9de992585..38c2169d7a 100644
> --- a/Documentation/git-check-ref-format.txt
> +++ b/Documentation/git-check-ref-format.txt
> @@ -88,7 +88,7 @@ but it is explicitly forbidden at the beginning of a branch
> name).
> When run with `--branch` option in a repository, the input is first
> expanded for the ``previous checkout syntax''
> `@{-n}`. For example, `@{-1}` is a way to refer the last thing that
> -was checked out using "git checkout" operation. This option should be
> +was checked out using "git switch-branch" operation. This option should be
> used by porcelains to accept this syntax anywhere a branch name is
> expected, so they can act as if you typed the branch name. As an
> exception note that, the ``previous checkout operation'' might result
> diff --git a/Documentation/git-format-patch.txt
> b/Documentation/git-format-patch.txt
> index aba4c5febe..0ceaa1173c 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -416,7 +416,7 @@ One way to test if your MUA is set up correctly is:
> * Apply it:
>
> $ git fetch master:test-apply
> -$ git checkout test-apply
> +$ git switch-branch test-apply
> $ git reset --hard
> $ git am a.patch
>
> diff --git a/Documentation/git-merge-base.txt
> b/Documentation/git-merge-base.txt
> index 9f07f4f6ed..1b25e5d530 100644
> ---
Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files
Nguyễn Thái Ngọc Duy writes:
> The good old "git checkout" command is still here and will be until
> all (or most of users) are sick of it.
Two comments on the goal (the implementation looked reasonable
assuming the reader agrees with the gaol).
At least to me, the verb "switch" needs two things to switch
between, i.e. "switch A and B", unless it is "switch to X".
Either "switch-to-branch" or simply "switch-to", perhaps?
As I already hinted in my response to Stefan (?) about
checkout-from-tree vs checkout-from-index, a command with multiple
modes of operation is not confusing to people with the right mental
model, and I suspect that having two separate commands for "checking
out a branch" and "checking out paths" that is done by this step
would help users to form the right mental model. So I tend to think
these two are "training wheels", and suspect that once they got it,
nobody will become "sick of" the single "checkout" command that can
be used to do either. It's just the matter of being aware what can
be done (which requires the right mental model) and how to tell Git
what the user wants it do (two separate commands, operating mode
option, or just the implied command line syntax---once the user
knows what s/he is doing, these do not make that much a difference).
As to the implementation,
> int dwim_new_local_branch;
> + int accept_pathspec;
> + int empty_arg_ok;
I think this is a reasonable way to completely avoid the codepath
that considers an unknown arg being parsed could be a pathspec
element (e.g. switch-to-that-branch mode will never want to see
pathspec, so disambiguation logic and/or hint should not kick in).
At the beginning of cmd_switch_branches(), please "explicitly"
assign 0 to accept_pathspec field, after memset(0) clears the whole
structure. When a reader sees memset(0), it is read as "giving a
clean and bland slate to futz with with a reasonable default", but
for this particular field, i.e. accept_pathspec, there is NO
reasoanble default. It's not like switch-to-branch mode is the heir
to checkout and the other sibling checkout-files is doing a
non-default behaviour.
Same comment probably would apply to the other one, although I
didn't think too deeply about that one.
> /*
>* If new checkout options are added, skip_merge_working_tree
> @@ -1056,7 +1068,7 @@ static int parse_branchname_arg(int argc, const char
> **argv,
> arg = argv[0];
> dash_dash_pos = -1;
> for (i = 0; i < argc; i++) {
> - if (!strcmp(argv[i], "--")) {
> + if (opts->accept_pathspec && !strcmp(argv[i], "--")) {
> dash_dash_pos = i;
> break;
> }
> @@ -1067,6 +1079,8 @@ static int parse_branchname_arg(int argc, const char
> **argv,
> has_dash_dash = 1; /* case (3) or (1) */
> else if (dash_dash_pos >= 2)
> die(_("only one reference expected, %d given."), dash_dash_pos);
> + else if (!opts->accept_pathspec)
> + has_dash_dash = 1;
>
> if (!strcmp(arg, "-"))
> arg = "@{-1}";
> @@ -1291,30 +1305,23 @@ static struct option
> *add_checkout_path_options(struct checkout_opts *opts,
> return newopts;
> }
>
> -int cmd_checkout(int argc, const char **argv, const char *prefix)
> +static int checkout_main(int argc, const char **argv, const char *prefix,
> + struct checkout_opts *opts, struct option *options,
> + const char * const usagestr[])
> {
> - struct checkout_opts real_opts;
> - struct checkout_opts *opts = _opts;
> struct branch_info new_branch_info;
> int dwim_remotes_matched = 0;
> - struct option *options = NULL;
>
> - memset(opts, 0, sizeof(*opts));
> memset(_branch_info, 0, sizeof(new_branch_info));
> opts->overwrite_ignore = 1;
> opts->prefix = prefix;
> opts->show_progress = -1;
> - opts->dwim_new_local_branch = 1;
>
> git_config(git_checkout_config, opts);
>
> opts->track = BRANCH_TRACK_UNSPECIFIED;
>
> - options = add_common_options(opts, options);
> - options = add_switch_branch_options(opts, options);
> - options = add_checkout_path_options(opts, options);
> -
> - argc = parse_options(argc, argv, prefix, options, checkout_usage,
> + argc = parse_options(argc, argv, prefix, options, usagestr,
>PARSE_OPT_KEEP_DASHDASH);
>
> if (opts->show_progress < 0) {
> @@ -1381,7 +1388,8 @@ int cmd_checkout(int argc, const char **argv, const
> char *prefix)
>_remotes_matched);
> argv += n;
> argc -= n;
> - }
> + } else if (!opts->empty_arg_ok)
> + usage_with_options(usagestr, options);
>
> if (argc) {
> parse_pathspec(>pathspec, 0,
> @@ -1443,3 +1451,60 @@ int cmd_checkout(int argc, const char **argv, const
> char
Re: [PATCH v2 1/7] parse-options: allow parse_options_concat(NULL, options)
Nguyễn Thái Ngọc Duy writes: > There is currently no caller that calls this function with "a" being > NULL. But it will be introduced shortly. It is used to construct the > option array from scratch, e.g. > >struct parse_options opts = NULL; Missing asterisk somewhere? >opts = parse_options_concat(opts, opts_1); >opts = parse_options_concat(opts, opts_2); > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > parse-options-cb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/parse-options-cb.c b/parse-options-cb.c > index 8c9edce52f..c609d52926 100644 > --- a/parse-options-cb.c > +++ b/parse-options-cb.c > @@ -126,7 +126,7 @@ struct option *parse_options_concat(struct option *a, > struct option *b) > struct option *ret; > size_t i, a_len = 0, b_len = 0; > > - for (i = 0; a[i].type != OPTION_END; i++) > + for (i = 0; a && a[i].type != OPTION_END; i++) > a_len++; > for (i = 0; b[i].type != OPTION_END; i++) > b_len++;
Re: [PATCH] tests: avoid syntax triggering old dash bug
Eric Sunshine writes:
> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
> wrote:
>> Avoid a bug in dash that's been fixed ever since its
>> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
>> released with dash v0.5.7 in July 2011.
>
> Perhaps enhance the commit message to explain the nature of the bug
> itself. It is not at all obvious from reading the above or from
> looking at the diff itself what the actual problem is that the patch
> is fixing. (And it wasn't even immediately obvious by looking at the
> commit message of ec2c84d in the dash repository.) To help readers of
> this patch avoid re-introducing this problem or diagnose such a
> failure, it might be a good idea to give an example of the syntax
> which trips up old dash (i.e. a here-doc followed immediately by a
> {...} expression) and the actual error message 'Syntax error: "}"
> unexpected'.
Indeed. From the patch text, I would not have even guessed. I was
wondering if there were interactions with "" and $() inside it.
If having {...} immediately after a here-doc is a problem, then the
patch should not touch existing code at all, but instead insert a
new line, perhaps like
: avoid open brace immediately after here-doc for old dash
immediately before {...}; that would have made it easier to grok.
>@@ -892,8 +892,9 @@ test_expect_success 'get --expiry-date' '
> 1510348087
> 0
> EOF
>+ date_valid1=$(git config --expiry-date date.valid1) &&
> {
>- echo "$rel_out $(git config --expiry-date date.valid1)"
>+ echo "$rel_out $date_valid1"
> git config --expiry-date date.valid2 &&
> git config --expiry-date date.valid3 &&
> git config --expiry-date date.valid4 &&
Re: [PATCH v1/RFC 1/1] 'git clone C:\cygwin\home\USER\repo' is working (again)
Johannes Schindelin writes:
> It takes a little folding and knotting of the brain to understand that
> this `!skip_dos_drive_prefix()` has *nothing* to do with the comment
> `unc paths` nor with the test whether the paths starts with two directory
> separators.
>
> As a consequence, I would highly suggest to turn this into:
>
> if (skip_dos_drive_prefix())
> ; /* absolute path with DOS drive prefix */
> /* unc paths */
> else if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
>
> That makes the code a lot easier to understand, and as a consequence a lot
> harder to mess up in the future.
Excellent. With or without "unc paths" comment, the separation does
make the logic more clear.
Re: [PATCH v1/RFC 1/1] 'git clone C:\cygwin\home\USER\repo' is working (again)
Johannes Schindelin writes: >> Sorry, but I fail to see the point the last example wants to make. > > I agree. For me, the real test is this: > > me@work ~ > $ cd /cygdrive > > me@work /cygdrive > $ ls > c d > > So `/cygdrive` *is* a valid directory in Cygwin. That supports the code that does not special case a path that begins with /cygdrive/ and simply treats it as a full path and freely use relative path, I guess. Very good point.
Re: [ANNOUNCE] Git v2.20.0-rc1
Johannes Schindelin writes: > ... > In short, even a thorough study of the code (keeping in mind the few > tidbits of information provided by you) leaves me really wondering which > code you run, because it sure does not look like current `master` to me. > > And if it is not `master`, then I have to ask why you keep suggesting to > turn off the built-in rebase by default in `master`. > > Ciao, > Johannes > > P.S.: Maybe you have a hook you forgot to mention? Any response? Or can I retract jc/postpone-rebase-in-c that was prepared as a reaction to this?
Re: [RFC PATCH] Introduce "precious" file concept
Ævar Arnfjörð Bjarmason writes: > What do you think about some patch like that which retains the plumbing > behavior for things like read-tree, doesn't introduce "precious" or > "trashable", and just makes you specify "[checkout|merge|...] --force" > in cases where we'd have clobbering? Whether you like it or not, don't people's automation use tons of invocations of "git merge", "git checkout", etc.? You'd be breaking them by such a change. Other than that, if we never had Git before and do not have to worry about existing users, I'd think it would be a lot closer to the ideal than today's system if "checkout foo.o" rejected overwriting "foo.o" that is not tracked in the current index but matches an ignore pattern, and required a "--force" option to overwrite it. A user, during a conflict resolution, may say "I want this 'git checkout foo/' to ignore conflicted paths in that directory, so I would give "--force" option to it, but now "--force" also implies that I am willing to clobber ignored paths, which means I cannot use it". I would think that a proper automation needs per-path hint from the user and/or the project, not just a single-size-fits-all --force option, and "unlike all the *.o ignored files that are expendable, this vendor-supplied-object.o is not" is one way to give such a per-path hint. > This would give scripts which relied on our stable plumbing consistent > behavior, while helping users who're using our main porcelain not to > lose data. I could then add a --force option to the likes of read-tree > (on by default), so you could get porcelain-like behavior with > --no-force. At that low level, I suspect that a single size fits all "--force" would work even less well.
Re: [PATCH] pack-protocol.txt: accept error packets in any context
Masaya Suzuki writes:
> In the Git pack protocol definition, an error packet may appear only in
> a certain context. However, servers can face a runtime error (e.g. I/O
> error) at an arbitrary timing. This patch changes the protocol to allow
> an error packet to be sent instead of any packet.
Makes perfect sense.
> Following this protocol spec change, the error packet handling code is
> moved to pkt-line.c.
>
> Signed-off-by: Masaya Suzuki
> ---
> Documentation/technical/pack-protocol.txt | 20 +++-
> builtin/archive.c | 2 --
> connect.c | 2 --
> fetch-pack.c | 2 --
> pkt-line.c| 4
> 5 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/technical/pack-protocol.txt
> b/Documentation/technical/pack-protocol.txt
> index 6ac774d5f..7a2375a55 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -22,6 +22,16 @@ protocol-common.txt. When the grammar indicate
> `PKT-LINE(...)`, unless
> otherwise noted the usual pkt-line LF rules apply: the sender SHOULD
> include a LF, but the receiver MUST NOT complain if it is not present.
>
> +An error packet is a special pkt-line that contains an error string.
> +
> +
> + error-line = PKT-LINE("ERR" SP explanation-text)
> +
> +
> +Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet
> MAY
> +be sent. Once this packet is sent by a client or a server, the data transfer
> +process defined in this protocol is terminated.
> +
> Transports
> --
> There are three transports over which the packfile protocol is
> @@ -89,13 +99,6 @@ process on the server side over the Git protocol is this:
> "0039git-upload-pack /schacon/gitbook.git\0host=example.com\0" |
> nc -v example.com 9418
>
> -If the server refuses the request for some reasons, it could abort
> -gracefully with an error message.
> -
> -
> - error-line = PKT-LINE("ERR" SP explanation-text)
> -
> -
>
> SSH Transport
> -
> @@ -398,12 +401,11 @@ from the client).
> Then the server will start sending its packfile data.
>
>
> - server-response = *ack_multi ack / nak / error-line
> + server-response = *ack_multi ack / nak
>ack_multi = PKT-LINE("ACK" SP obj-id ack_status)
>ack_status = "continue" / "common" / "ready"
>ack = PKT-LINE("ACK" SP obj-id)
>nak = PKT-LINE("NAK")
> - error-line = PKT-LINE("ERR" SP explanation-text)
>
>
> A simple clone may look like this (with no 'have' lines):
> diff --git a/builtin/archive.c b/builtin/archive.c
> index d2455237c..5d179bbd1 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -59,8 +59,6 @@ static int run_remote_archiver(int argc, const char **argv,
> if (strcmp(buf, "ACK")) {
> if (starts_with(buf, "NACK "))
> die(_("git archive: NACK %s"), buf + 5);
> - if (starts_with(buf, "ERR "))
> - die(_("remote error: %s"), buf + 4);
> die(_("git archive: protocol error"));
> }
>
> diff --git a/connect.c b/connect.c
> index 24281b608..458906e60 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -306,8 +306,6 @@ struct ref **get_remote_heads(struct packet_reader
> *reader,
> die_initial_contact(1);
> case PACKET_READ_NORMAL:
> len = reader->pktlen;
> - if (len > 4 && skip_prefix(reader->line, "ERR ", ))
> - die(_("remote error: %s"), arg);
> break;
> case PACKET_READ_FLUSH:
> state = EXPECTING_DONE;
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 9691046e6..e66cd7b71 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -178,8 +178,6 @@ static enum ack_type get_ack(int fd, struct object_id
> *result_oid)
> return ACK;
> }
> }
> - if (skip_prefix(line, "ERR ", ))
> - die(_("remote error: %s"), arg);
> die(_("git fetch-pack: expected ACK/NAK, got '%s'"), line);
> }
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 04d10bbd0..ce9e42d10 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd,
> char **src_buffer,
> return PACKET_READ_EOF;
> }
>
> + if (starts_with(buffer, "ERR ")) {
> + die(_("remote error: %s"), buffer + 4);
> + }
> +
> if ((options & PACKET_READ_CHOMP_NEWLINE) &&
> len && buffer[len-1] == '\n')
> len--;
Re: [PATCH v1/RFC 1/1] 'git clone C:\cygwin\home\USER\repo' is working (again)
Steven Penny writes: > If you strip the drive, you can still navigate within the same drive: > > $ cd 'C:\Users' > $ pwd > /cygdrive/c/Users > > $ cd '\Windows' > $ pwd > /cygdrive/c/Windows > > but you can no longer traverse drives: > > $ cd '\Testing' > sh: cd: \Testing: No such file or directory Sorry, but I fail to see the point the last example wants to make. If it were $ cd /cygdrive/d/Testing $ cd /cygdrive/c/Users $ cd ../../d/Testing and the last step fails, then I would suspect it would make sense to treat /cygdrive/$X exactly like how we would treat $C:, because $ cd C:Users $ cd ../D:Testing would not make sense, either, which is an indication that these two are quite similar. On the other hand, if "cd ../../d/Testing" above does not fail and does what non-DOS users would expect, then that strongly argues that treating /cygdrive/$X any specially is a mistake. > So a good first question for me would be: why are we stripping "C:" or similar > in the first place? Sorry, but I do not see the connection to this question and the above example. The reason why we strip C: is because the letter that comes after that colon determines if we are talking about absolute path (in other words, the current directory does not play a role in determining which directory the path refers to), unlike the POSIX codepath where it is always the first letter in the pathname. C:\Users is a directory whose name is Users at the top level of the C: drive. C0\Users tells us that in the current directory, there is a directory whose name is C0 and in it, there is a filesystem entity whose name is Users. So the colon that follows an alpha (in this case, C:) is quite special, compared to other letters (in this example, I used '0' to contrast its effect with ':'). So it is very understandable why we want to have has_dos_prefix() and skip_dos_prefix(). > I would say these could be merged into a "win.h" or similar. Cygwin typically > leans toward the "/unix/style" while MINGW has been more tolerant of > "C:\Windows\Style" and "C:/Mixed/Style" paths, i dont see that changing. I'd defer to Windows folks to decide if a unified win.h is a good idea. Thanks.
Re: [PATCH v1/RFC 1/1] 'git clone C:\cygwin\home\USER\repo' is working (again)
tbo...@web.de writes:
> Reported-By: Steven Penny
> Signed-off-by: Torsten Bögershausen
> ---
>
> This is the first vesion of a patch.
> Is there a chance that you test it ?
>
> abspath.c | 2 +-
> compat/cygwin.c | 18 ++
> compat/cygwin.h | 32
> 3 files changed, 47 insertions(+), 5 deletions(-)
I am hoping that the funny indentation above is merely an accidental
touch on the delete key and not a sign of MUA eating the patch to
make it unapplicable (and making it harder for those who want to
test to test it).
> diff --git a/compat/cygwin.c b/compat/cygwin.c
> index b9862d606d..c4a10cb5a1 100644
> --- a/compat/cygwin.c
> +++ b/compat/cygwin.c
> @@ -1,19 +1,29 @@
> #include "../git-compat-util.h"
> #include "../cache.h"
>
> +int cygwin_skip_dos_drive_prefix(char **path)
> +{
> + int ret = has_dos_drive_prefix(*path);
> + *path += ret;
> + return ret;
> +}
Mental note: this is exactly the same as mingw version.
I wonder if it makes the rest of the code simpler if we stripped
things like /cygdrive/c here exactly the sam way as we strip C:
For that, has_dos_drive_prefix() needs to know /cygdrive/[a-z],
which may not be a bad thing, I guess. Let's read on.
> int cygwin_offset_1st_component(const char *path)
> {
> - const char *pos = path;
> + char *pos = (char *)path;
> +
> /* unc paths */
> - if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
> + if (!skip_dos_drive_prefix() &&
> + is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
When given C:\foo\bar, this strips prefix to leave \foo\bar in pos and
then realizes that it cannot be unc path (because it has dos prefix)
and goes on. What is returned from the function is "\foo\bar" + 1 -
path, i.e. the offset in the original "C:\foo\bar" string of the
'f' in "foo", i.e. 3.
When given \foo\bar, pos stays the same as path, and it skips the
first backslash and returns the offset in the original string of the
'f' in "foo", i.e. 1.
Both cases return the moreal equivalent --- the offset of the first
component 'foo'. So this looks correct for these two cases.
> /* skip server name */
> - pos = strchr(pos + 2, '/');
> + pos = strpbrk(pos + 2, "\\/");
This is to allow \\server\path in addition to //server/path; the
original looked only for '/' with strchr but we now look for either
'/' or '\', whichever comes earlier. Both helpers return NULL when
they find no separator, so we should be able to handle the returned
pos from here on the same way as the original code.
> if (!pos)
> return 0; /* Error: malformed unc path */
>
> do {
> pos++;
> - } while (*pos && pos[0] != '/');
> + } while (*pos && !is_dir_sep(*pos));
And whenever we looked for '/', we consider '\' its equivalent.
> }
> +
> return pos + is_dir_sep(*pos) - path;
> }
Looks good so far.
Wait, did I just waste time by not looking at mingw.c version? I
suspect this would be exactly the same ;-)
> diff --git a/compat/cygwin.h b/compat/cygwin.h
> index 8e52de4644..46f29c0a90 100644
> --- a/compat/cygwin.h
> +++ b/compat/cygwin.h
> @@ -1,2 +1,34 @@
> +#define has_dos_drive_prefix(path) \
> + (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
Metanl note: this also looks the same as mingw version.
> +int cygwin_offset_1st_component(const char *path);
> +#define offset_1st_component cygwin_offset_1st_component
> +
So, my real questions are
- Is there a point in having cygwin specific variant of these, or
can we just borrow from mingw version (with some refactoring)?
Is there a point in doing so (e.g. if mingw plans to move to
reject forward slashes, attempting to share is pointless).
- Would it make it better (or worse) to treat the /cygdrive/c thing
as another way to spell dos-drive-prefix? If the answer is "it
is a good idea", then that answers the previous question
automatically (we cannot gain much by sharing, as mingw side
won't want to treat /cygdrive/c any differently).
Re: [PATCH v1 1/2] log -G: Ignore binary files
Stefan Beller writes: > On Wed, Nov 21, 2018 at 1:08 PM Thomas Braun > wrote: >> >> The -G option of log looks for the differences whose patch text >> contains added/removed lines that match regex. >> >> The concept of differences only makes sense for text files, therefore >> we need to ignore binary files when searching with -G as well. > > What about partial text/partial binary files? Good point. You'd use "-a" (or "--text") to tell the diff machinery to treat the contents as text, and the new logic must pay attention to that command line option.
Re: [PATCH 2/5] ieot: default to not writing IEOT section
Stefan Beller writes:
>> > +static int record_ieot(void)
>> > +{
>> > + int val;
>> > +
>>
>> Initialize stack val to zero to ensure proper default.
>
> I don't think that is needed here, as we only use `val` when
> we first write to it via git_config_get_bool.
Yup.
