* Re: There are too many unreachable loose objects
From: Jeff King @ 2017-02-16 22:57 UTC (permalink / raw)
To: Jacob Keller; +Cc: Hilco Wijbenga, Git Users
In-Reply-To: <CA+P7+xqWoUBOoFSiOTT5U-9aoqESUMnZeSGfvhGte2LqF18gmw@mail.gmail.com>
On Thu, Feb 16, 2017 at 02:36:10PM -0800, Jacob Keller wrote:
> > Whenever I run "git push --force(-with-lease)" I get a variation of
> >
> > Counting objects: 187, done.
> > Delta compression using up to 12 threads.
> > Compressing objects: 100% (126/126), done.
> > Writing objects: 100% (187/187), 21.35 KiB | 0 bytes/s, done.
> > Total 187 (delta 78), reused 71 (delta 20)
> > warning: There are too many unreachable loose objects; run 'git prune'
> > to remove them.
> > To git@git.company.com:project.git
> > + 51338ea...b0ebe39 my-branch -> my-branch (forced update)
> >
> > So I'll run "git prune" and, for good measure, "git gc" (which even
> > includes "git prune"?). The first seems to do nothing, the latter does
> > its thing.
> >
>
> It may be that it's the server side that needs to git-prune, and not
> your local side? I'm not really certain but you're doing a push which
> talks to a remote server.
Yes, certainly the position in the output implies that. These days you
should see:
remote: warning: There are too many...
to make it more clear. Perhaps the server is too old to have 860a2ebec
(receive-pack: send auto-gc output over sideband 2, 2016-06-05).
-Peff
^ permalink raw reply
* Re: [RFCv4 PATCH 00/14] Checkout aware of Submodules!
From: Stefan Beller @ 2017-02-16 22:56 UTC (permalink / raw)
To: Jacob Keller
Cc: Junio C Hamano, Git mailing list, brian m. carlson,
Jonathan Nieder, Brandon Williams
In-Reply-To: <CA+P7+xpWqiWeMOMauYgtVaiUngd73_dUqRxyWcp0QPHvgOe+vg@mail.gmail.com>
On Thu, Feb 16, 2017 at 2:54 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Thu, Feb 16, 2017 at 2:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> Integrate updating the submodules into git checkout,...
>>
>> It was more or less a pleasant read, once I decided to pretend that
>> I were a machine who uses identifiers only to identify locations in
>> the program ;-) IOW, for human consumption, the new names introduced
>> were sometimes quite confusing and went against helping understanding.
>>
>
> Based on my cursory reading, I agree. I had trouble understanding how
> the process worked because the names were somewhat confusing. They
> started to make some sense as I read more. I think v4 had better names
> than v3, but they were still somewhat confusing to me.
>
Now if only you could tell me what names were better to understand. ;)
I'll reply to the individual patch remarks and hopefully there we find
good names for these functions.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v2 00/16] Remove submodule from files-backend.c
From: Stefan Beller @ 2017-02-16 22:55 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git@vger.kernel.org, Junio C Hamano, Michael Haggerty,
Johannes Schindelin, Ramsay Jones, David Turner
In-Reply-To: <20170216114818.6080-1-pclouds@gmail.com>
This series looks good to me.
Thanks,
Stefan
^ permalink raw reply
* Re: [RFCv4 PATCH 00/14] Checkout aware of Submodules!
From: Jacob Keller @ 2017-02-16 22:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Git mailing list, brian m. carlson,
Jonathan Nieder, Brandon Williams
In-Reply-To: <xmqqlgt5vlse.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 16, 2017 at 2:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Integrate updating the submodules into git checkout,...
>
> It was more or less a pleasant read, once I decided to pretend that
> I were a machine who uses identifiers only to identify locations in
> the program ;-) IOW, for human consumption, the new names introduced
> were sometimes quite confusing and went against helping understanding.
>
Based on my cursory reading, I agree. I had trouble understanding how
the process worked because the names were somewhat confusing. They
started to make some sense as I read more. I think v4 had better names
than v3, but they were still somewhat confusing to me.
Regards,
Jake
^ permalink raw reply
* Re: There are too many unreachable loose objects
From: Jacob Keller @ 2017-02-16 22:36 UTC (permalink / raw)
To: Hilco Wijbenga; +Cc: Git Users
In-Reply-To: <CAE1pOi3bpL0zTS9w2QCOrXcWk5pHj=xthxo2nYi8KEwJ=TgXfA@mail.gmail.com>
On Thu, Feb 16, 2017 at 1:58 PM, Hilco Wijbenga
<hilco.wijbenga@gmail.com> wrote:
> Hi all,
>
> Whenever I run "git push --force(-with-lease)" I get a variation of
>
> Counting objects: 187, done.
> Delta compression using up to 12 threads.
> Compressing objects: 100% (126/126), done.
> Writing objects: 100% (187/187), 21.35 KiB | 0 bytes/s, done.
> Total 187 (delta 78), reused 71 (delta 20)
> warning: There are too many unreachable loose objects; run 'git prune'
> to remove them.
> To git@git.company.com:project.git
> + 51338ea...b0ebe39 my-branch -> my-branch (forced update)
>
> So I'll run "git prune" and, for good measure, "git gc" (which even
> includes "git prune"?). The first seems to do nothing, the latter does
> its thing.
>
It may be that it's the server side that needs to git-prune, and not
your local side? I'm not really certain but you're doing a push which
talks to a remote server.
Thanks,
Jake
> And then the next time (which could be a few minutes later) I get the
> same warning. My branches aren't that big, the largest ever had 40-ish
> commits. So abandoning a few dozen commits should not lead to this
> warning, I would think.
>
> What am I doing wrong?
>
> Cheers,
> Hilco
^ permalink raw reply
* Re: [PATCH 08/15] submodules: introduce check to see whether to touch a submodule
From: Jacob Keller @ 2017-02-16 22:34 UTC (permalink / raw)
To: Stefan Beller
Cc: Git mailing list, brian m. carlson, Jonathan Nieder,
Brandon Williams, Junio C Hamano
In-Reply-To: <20170216003811.18273-9-sbeller@google.com>
On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller <sbeller@google.com> wrote:
> +int touch_submodules_in_worktree(void)
> +{
> + /*
> + * Update can't be "none", "merge" or "rebase",
> + * treat any value as OFF, except an explicit ON.
> + */
> + return config_update_recurse_submodules == RECURSE_SUBMODULES_ON;
> +}
> +
This function doesn't and the comment don't make sense to me. What do
you mean update can't be "none", "merge", or "rebase"? I'm thinking
this means that the update_recurse_submodules checks whether it's ok
for doing recursive update on submodules but only when the update type
is checkout? This appears to be connected directly to the previous
patch that reads the config value somehow. This is pretty convoluted
to me, and took me quite a while to understand. Is it possible to make
this more clear in the comments or in the name?
^ permalink raw reply
* Re: [RFCv4 PATCH 00/14] Checkout aware of Submodules!
From: Junio C Hamano @ 2017-02-16 22:00 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, sandals, jrnieder, bmwill
In-Reply-To: <20170216003811.18273-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Integrate updating the submodules into git checkout,...
It was more or less a pleasant read, once I decided to pretend that
I were a machine who uses identifiers only to identify locations in
the program ;-) IOW, for human consumption, the new names introduced
were sometimes quite confusing and went against helping understanding.
I saw a few places where logic looked somewhat iffy, which I sent
separate comments on; I may spot more if the code used more
understandable names and calling conventions, but that is OK. It is
an expected part of an iterative process.
I can feel that this topic is getting closer to where we eventually
want to go.
Thanks.
^ permalink raw reply
* There are too many unreachable loose objects
From: Hilco Wijbenga @ 2017-02-16 21:58 UTC (permalink / raw)
To: Git Users
Hi all,
Whenever I run "git push --force(-with-lease)" I get a variation of
Counting objects: 187, done.
Delta compression using up to 12 threads.
Compressing objects: 100% (126/126), done.
Writing objects: 100% (187/187), 21.35 KiB | 0 bytes/s, done.
Total 187 (delta 78), reused 71 (delta 20)
warning: There are too many unreachable loose objects; run 'git prune'
to remove them.
To git@git.company.com:project.git
+ 51338ea...b0ebe39 my-branch -> my-branch (forced update)
So I'll run "git prune" and, for good measure, "git gc" (which even
includes "git prune"?). The first seems to do nothing, the latter does
its thing.
And then the next time (which could be a few minutes later) I get the
same warning. My branches aren't that big, the largest ever had 40-ish
commits. So abandoning a few dozen commits should not lead to this
warning, I would think.
What am I doing wrong?
Cheers,
Hilco
^ permalink raw reply
* Re: [PATCH 10/15] update submodules: add submodule_go_from_to
From: Stefan Beller @ 2017-02-16 21:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, brian m. carlson, Jonathan Nieder,
Brandon Williams
In-Reply-To: <xmqq7f4px2fi.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 16, 2017 at 1:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> [administrivia: I've been seeing "unlisted-recipients:; (no To-header on input)"
> for all of your recent patches. Can it be corrected on your end, please?]
I cc'd everyone and had no to field, actually. Maybe git-send-email should
complain more loudly when I am holding it wrong.
>> + cp.git_cmd = 1;
>> + argv_array_pushl(&cp.args, "diff-index", "--cached", "HEAD", NULL);
>
> We'd want to use the QUICK optimization here, I suspect. This
> caller does not need to (or want to) learn which exact paths are
> modified, right?
ok
>> + if (run_command(&cp))
>> + die("could not clean submodule index");
>> +}
>
> Do s/clean/reset/ everywhere above.
ok
>
>> +/**
>> + * Moves a submodule at a given path from a given head to another new head.
>> + * For edge cases (a submodule coming into existence or removing a submodule)
>> + * pass NULL for old or new respectively.
>> + *
>> + * TODO: move dryrun and forced to flags.
>
> The reason why this seeingly trivial thing is left as TODO is...???
will do. The reason was to first get the grand design right before
spending more time in the details.
>
>> + */
>> +int submodule_go_from_to(const char *path,
>> + const char *old,
>> + const char *new,
>> + int dry_run,
>> + int force)
>> +{
>
> go-from-to does not tell me what it does, but my cursory read of the
> body of the function tells me that this is doing a checkout of a
> branch in the submodule? The operation in builtin/checkout.c that
> conceptually correspond to this is called switch_branches(), I
> think, so perhaps submodule_switch_branches() is a better name?
Well as of now all submodule operations (submodule update mostly)
end up with detached HEADs in the submodule. So it is rather
going from one state (sha1) to another given sha1.
I would rather compare it to checkout_entry/write_entry in entry.c
except that there are more things to go wrong. A single file has no
notion of its own index or dirtiness.
>
>> + int ret = 0;
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> + const struct submodule *sub;
>> +
>> + sub = submodule_from_path(null_sha1, path);
>> +
>> + if (!sub)
>> + die("BUG: could not get submodule information for '%s'", path);
>> +
>> + if (!dry_run) {
>> + if (old) {
>> + if (!submodule_uses_gitfile(path))
>> + absorb_git_dir_into_superproject("", path,
>> + ABSORB_GITDIR_RECURSE_SUBMODULES);
>> + } else {
>> + struct strbuf sb = STRBUF_INIT;
>> + strbuf_addf(&sb, "%s/modules/%s",
>> + get_git_common_dir(), sub->name);
>> + connect_work_tree_and_git_dir(path, sb.buf);
>> + strbuf_release(&sb);
>> +
>> + /* make sure the index is clean as well */
>> + submodule_clean_index(path);
>> + }
>> + }
>> +
>> + if (old && !force) {
>> + /* Check if the submodule has a dirty index. */
>> + if (submodule_has_dirty_index(sub)) {
>> + /* print a thing here? */
>> + return -1;
>> + }
>
> Isn't it too late to do this here? You already reset the index
> in the submodule, no?
Yes this is confusing.
We run this function first as a dry_run, and in a second pass
as a real run. So the order inside the function is confusing
as we would run this first in the dry run.
>
> Is the idea that changes that are only in the submodule's working
> tree are noticed by later "read-tree -u -m" down there? Not
> complaining but trying to understand.
I think (as a first step) we only want to allow a clean index in
submodules, as then we have to implement less cases at first.
^ permalink raw reply
* Re: [PATCH 13/15] read-cache: remove_marked_cache_entries to wipe selected submodules.
From: Junio C Hamano @ 2017-02-16 21:32 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, sandals, jrnieder, bmwill
In-Reply-To: <20170216003811.18273-14-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
The title was ungrokkable to me, but after reading the code I think
you are teaching the normal codepath where removal of working tree
is done to match what is done to the index that submodules are also
in the working tree and need to be removed when the corresopnding ce
is removed.
Which makes sense.
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> read-cache.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index 9054369dd0..b78a7f02e3 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -18,6 +18,7 @@
> #include "varint.h"
> #include "split-index.h"
> #include "utf8.h"
> +#include "submodule.h"
>
> /* Mask for the name length in ce_flags in the on-disk index */
>
> @@ -532,6 +533,8 @@ void remove_marked_cache_entries(struct index_state *istate)
>
> for (i = j = 0; i < istate->cache_nr; i++) {
> if (ce_array[i]->ce_flags & CE_REMOVE) {
> + if (is_active_submodule_with_strategy(ce_array[i], SM_UPDATE_UNSPECIFIED))
> + submodule_go_from_to(ce_array[i]->name, "HEAD", NULL, 0, 1);
> remove_name_hash(istate, ce_array[i]);
> save_or_free_index_entry(istate, ce_array[i]);
> }
^ permalink raw reply
* [PATCH] tempfile: avoid "ferror | fclose" trick
From: Jeff King @ 2017-02-16 21:31 UTC (permalink / raw)
To: Andreas Schwab
Cc: Michael Haggerty, Junio C Hamano, Jáchym Barvínek, git
In-Reply-To: <20170216164359.k2ab7laqqvusfsm2@sigill.intra.peff.net>
On Thu, Feb 16, 2017 at 11:43:59AM -0500, Jeff King wrote:
> On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:
>
> > >> int xfclose(FILE *fp)
> > >> {
> > >> return ferror(fp) | fclose(fp);
> > >> }
> > >
> > > Yes, that's exactly what I had in mind (might be worth calling out the
> > > bitwise-OR, though, just to make it clear it's not a typo).
> >
> > Since the order of evaluation is unspecified, it would be better to
> > force sequencing ferror before fclose.
>
> Good point. Arguably the call in tempfile.c is buggy.
Here's a fix.
I think close_tempfile() suffers from the same errno problem discussed
earlier in this thread (i.e., that after calling it, you may get an
error return with a random, unrelated errno value if ferror() failed but
fclose() did not).
-- >8 --
Subject: [PATCH] tempfile: avoid "ferror | fclose" trick
The current code wants to record an error condition from
either ferror() or fclose(), but makes sure that we always
call both functions. So it can't use logical-OR "||", which
would short-circuit when ferror() is true. Instead, it uses
bitwise-OR "|" to evaluate both functions and set one or
more bits in the "err" flag if they reported a failure.
Unlike logical-OR, though, bitwise-OR does not introduce a
sequence point, and the order of evaluation for its operands
is unspecified. So a compiler would be free to generate code
which calls fclose() first, and then ferror() on the
now-freed filehandle.
There's no indication that this has happened in practice,
but let's write it out in a way that follows the standard.
Noticed-by: Andreas Schwab <schwab@linux-m68k.org>
Signed-off-by: Jeff King <peff@peff.net>
---
tempfile.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/tempfile.c b/tempfile.c
index 2990c9242..ffcc27237 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -247,12 +247,8 @@ int close_tempfile(struct tempfile *tempfile)
tempfile->fd = -1;
if (fp) {
tempfile->fp = NULL;
-
- /*
- * Note: no short-circuiting here; we want to fclose()
- * in any case!
- */
- err = ferror(fp) | fclose(fp);
+ err = ferror(fp);
+ err |= fclose(fp);
} else {
err = close(fd);
}
--
2.12.0.rc1.559.gd292418bf
^ permalink raw reply related
* Re: [PATCH v3] reset: add an example of how to split a commit into two
From: Jacob Keller @ 2017-02-16 21:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <xmqqpoihx6f4.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 16, 2017 at 11:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jacob Keller <jacob.e.keller@intel.com> writes:
>>
>>> The interdiff between v2 and v3 is not really worth showing since I
>>> basically re-wrote the entire section a bit.
>>
>> Could this be made into an incremental, now that v2 has been in
>> 'next' for about 10 days, please?
>
> Nah, I think it is easier to read "log -p" if I just revert v2 out
> of existence from 'next', and queue this (with a minor typofixes) as
> a different topic to be merged later to 'master'.
>
Ok. Yea, I didn't even realize it was put into next because of the
various comments I'd received. I guess I could have checked, but the
diff really is bad when seeing incrementally.
> So no need to resend and certainly no need to make it incremental.
>
>>> +Split a commit apart into a sequence of commits::
>>> ++
>>> +Suppose that you have create lots of logically separate changes and commit them
>>
>> s/create/&d/; s/commit/&ed/
>
> I'd do this myself while queuing.
>
> Thanks.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 05/15] connect_work_tree_and_git_dir: safely create leading directories
From: Junio C Hamano @ 2017-02-16 21:25 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, brian m. carlson, Jonathan Nieder,
Brandon Williams
In-Reply-To: <CAGZ79kbMmPQ1ZxtdaoFof36g1An-xsAx85etiskirJfY4tGHaQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> The above does somewhat more than advertised and was a bit hard to
>> grok. Initially I thought the reason why pathdup()s were delayed
>> was perhaps because you pathdup() something potentially different
>> from the given parameter to the function (i.e. new code before
>> pathdup() may tweak what is pathdup()ed).
>>
>> But that is not what is happening. I suspect that you did this to
>> avoid leaking allocated memory when the code calls die().
>
> That is not what is happening, either.
That's a good sign that you need a bit more in the log message.
^ permalink raw reply
* Re: [PATCH 12/15] unpack-trees: check if we can perform the operation for submodules
From: Junio C Hamano @ 2017-02-16 21:23 UTC (permalink / raw)
To: Stefan Beller
In-Reply-To: <20170216003811.18273-13-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> +
> + /* ERROR_WOULD_LOSE_UNTRACKED_SUBMODULE */
> + "Submodule '%s' cannot be deleted as it contains untracked files.",
OK.
> + msgs[ERROR_WOULD_LOSE_UNTRACKED_SUBMODULE] =
> + _("Submodule '%s' cannot be deleted as it contains untracked files.");
OK again.
> @@ -240,12 +246,44 @@ static void display_error_msgs(struct unpack_trees_options *o)
> fprintf(stderr, _("Aborting\n"));
> }
>
> +static int submodule_check_from_to(const struct cache_entry *ce, const char *old_id, const char *new_id, struct unpack_trees_options *o)
> +{
> + if (submodule_go_from_to(ce->name, old_id,
> + new_id, 1, o->reset))
> + return o->gently ? -1 :
> + add_rejected_path(o, ERROR_WOULD_LOSE_UNTRACKED_SUBMODULE, ce->name);
Is potential loss of untracked paths the only reason
submodule_go_from_to() would fail? I somehow thought that it would
not even care about untracked paths but cared deeply about already
added changes.
^ permalink raw reply
* Re: [PATCH 11/15] unpack-trees: pass old oid to verify_clean_submodule
From: Junio C Hamano @ 2017-02-16 21:19 UTC (permalink / raw)
To: Stefan Beller
In-Reply-To: <20170216003811.18273-12-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> The check (which uses the old oid) is yet to be implemented, but this part
> is just a refactor, so it can go separately first.
If this didn't pass an unused parameter, then the change is just a
refactor, and I do think such a "just a refactor" can be a good step
on its own to keep the future step to manageable complexity.
With an unused parameter being passed, I do not think it is a good
logical single step anymore, though.
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> unpack-trees.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 3a8ee19fe8..616a0ae4b2 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1407,7 +1407,8 @@ static void invalidate_ce_path(const struct cache_entry *ce,
> * Currently, git does not checkout subprojects during a superproject
> * checkout, so it is not going to overwrite anything.
> */
> -static int verify_clean_submodule(const struct cache_entry *ce,
> +static int verify_clean_submodule(const char *old_sha1,
> + const struct cache_entry *ce,
> enum unpack_trees_error_types error_type,
> struct unpack_trees_options *o)
> {
> @@ -1427,16 +1428,18 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
> struct dir_struct d;
> char *pathbuf;
> int cnt = 0;
> - unsigned char sha1[20];
>
> - if (S_ISGITLINK(ce->ce_mode) &&
> - resolve_gitlink_ref(ce->name, "HEAD", sha1) == 0) {
> - /* If we are not going to update the submodule, then
> + if (S_ISGITLINK(ce->ce_mode)) {
> + unsigned char sha1[20];
> + int sub_head = resolve_gitlink_ref(ce->name, "HEAD", sha1);
> + /*
> + * If we are not going to update the submodule, then
> * we don't care.
> */
> - if (!hashcmp(sha1, ce->oid.hash))
> + if (!sub_head && !hashcmp(sha1, ce->oid.hash))
> return 0;
> - return verify_clean_submodule(ce, error_type, o);
> + return verify_clean_submodule(sub_head ? NULL : sha1_to_hex(sha1),
> + ce, error_type, o);
> }
>
> /*
^ permalink raw reply
* Re: [PATCH 05/15] connect_work_tree_and_git_dir: safely create leading directories
From: Stefan Beller @ 2017-02-16 21:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, brian m. carlson, Jonathan Nieder,
Brandon Williams
In-Reply-To: <xmqqh93tx3f3.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 16, 2017 at 12:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> In a later patch we'll use connect_work_tree_and_git_dir when the
>> directory for the gitlink file doesn't exist yet. Safely create
>> the directory first.
>>
>> One of the two users of 'connect_work_tree_and_git_dir' already checked
>> for the directory being there, so we can loose that check.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> dir.c | 32 +++++++++++++++++++++-----------
>> submodule.c | 11 ++---------
>> 2 files changed, 23 insertions(+), 20 deletions(-)
>>
>> diff --git a/dir.c b/dir.c
>> index 4541f9e146..6f52af7abb 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -2728,23 +2728,33 @@ void untracked_cache_add_to_index(struct index_state *istate,
>> /* Update gitfile and core.worktree setting to connect work tree and git dir */
>> void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
>> {
>> - struct strbuf file_name = STRBUF_INIT;
>> + struct strbuf gitfile_sb = STRBUF_INIT;
>> + struct strbuf cfg_sb = STRBUF_INIT;
>> struct strbuf rel_path = STRBUF_INIT;
>> - char *git_dir = real_pathdup(git_dir_);
>> - char *work_tree = real_pathdup(work_tree_);
>> + char *git_dir, *work_tree;
>>
>> - /* Update gitfile */
>> - strbuf_addf(&file_name, "%s/.git", work_tree);
>> - write_file(file_name.buf, "gitdir: %s",
>> - relative_path(git_dir, work_tree, &rel_path));
>> + /* Prepare .git file */
>> + strbuf_addf(&gitfile_sb, "%s/.git", work_tree_);
>> + if (safe_create_leading_directories_const(gitfile_sb.buf))
>> + die(_("could not create directories for %s"), gitfile_sb.buf);
>> +
>> + /* Prepare config file */
>> + strbuf_addf(&cfg_sb, "%s/config", git_dir_);
>> + if (safe_create_leading_directories_const(cfg_sb.buf))
>> + die(_("could not create directories for %s"), cfg_sb.buf);
>>
>> + git_dir = real_pathdup(git_dir_);
>> + work_tree = real_pathdup(work_tree_);
>> +
>> + /* Write .git file */
>> + write_file(gitfile_sb.buf, "gitdir: %s",
>> + relative_path(git_dir, work_tree, &rel_path));
>
> The above does somewhat more than advertised and was a bit hard to
> grok. Initially I thought the reason why pathdup()s were delayed
> was perhaps because you pathdup() something potentially different
> from the given parameter to the function (i.e. new code before
> pathdup() may tweak what is pathdup()ed).
>
> But that is not what is happening. I suspect that you did this to
> avoid leaking allocated memory when the code calls die().
That is not what is happening, either.
AFAICT real_pathdup resolves a path to its real path. But the path
*must* exist already (except the very last part, i.e. the file name).
So the SCLD must come before the real_pathdup, which has to come
before its consumers, which are the relative_path calls for write_file
as well as git_config_set_in_file.
So structurally we need to:
1a) construct $GIT_DIR/modules/<name>/config
1b) SCLD 1a)
1c) get absolute path of 1a)
2a) construct <submodule path>/.git file
2b) SCLD 2a)
2c) get absolute path of 2a)
3) compute relative path of 1c and 2c
both ways, write to appropriate places.
I chose to structure it as:
1a
1b
2a
2b
1c
2c
3
because the a/b steps seemed very related and the order is valid.
>
> If the code was written like so from the beginning, I do not see a
> reason to move the pathdup() up to deliberately make it leak [*1*].
> But as a part of this change, I found it distracting and getting in
> the way of understanding the change. If you really care, it is
> nicer to do it to reviewers as a separate preparatory clean-up step,
> or follow-up standalone clean-up patch after the series settles.
I considered doing it in multiple patches as we have different things
going on here:
1) as the commit claims have SCLD in connect_work_tree_and_git_dir
2) keep the dependencies of SCLD and real_pathdup in order. C.f.
the deleted lines of code in submodule.c
> It is a good thing to do to make sure git_dir_ exists and it should
> be mentioned in the log message. Adding "Do the same for the place
> where the per-repo config file is created". at the end of the first
> paragraph should be sufficient.
>
will do.
Thanks for thorough review!
Stefan
^ permalink raw reply
* Re: [PATCH 10/15] update submodules: add submodule_go_from_to
From: Junio C Hamano @ 2017-02-16 21:15 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, sandals, jrnieder, bmwill
In-Reply-To: <20170216003811.18273-11-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
[administrivia: I've been seeing "unlisted-recipients:; (no To-header on input)"
for all of your recent patches. Can it be corrected on your end, please?]
> In later patches we introduce the options and flag for commands
> that modify the working directory, e.g. git-checkout.
>
> This piece of code will be used universally for
> all these working tree modifications as it
> * supports dry run to answer the question:
> "Is it safe to change the submodule to this new state?"
> e.g. is it overwriting untracked files or are there local
> changes that would be overwritten?
> * supports a force flag that can be used for resetting
> the tree.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> submodule.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> submodule.h | 5 ++
> 2 files changed, 156 insertions(+)
>
> diff --git a/submodule.c b/submodule.c
> index b262c5b0ad..84cc62f3bb 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1250,6 +1250,157 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
> return ret;
> }
>
> +static int submodule_has_dirty_index(const struct submodule *sub)
> +{
> + ssize_t len;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf buf = STRBUF_INIT;
> + int ret = 0;
> +
> + prepare_submodule_repo_env_no_git_dir(&cp.env_array);
> +
> + cp.git_cmd = 1;
> + argv_array_pushl(&cp.args, "diff-index", "--cached", "HEAD", NULL);
We'd want to use the QUICK optimization here, I suspect. This
caller does not need to (or want to) learn which exact paths are
modified, right?
> +void submodule_clean_index(const char *path)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + prepare_submodule_repo_env_no_git_dir(&cp.env_array);
> +
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> +
> + argv_array_pushf(&cp.args, "--super-prefix=%s/", path);
> + argv_array_pushl(&cp.args, "read-tree", "-u", "--reset", NULL);
> +
> + argv_array_push(&cp.args, EMPTY_TREE_SHA1_HEX);
> +
> + if (run_command(&cp))
> + die("could not clean submodule index");
> +}
Do s/clean/reset/ everywhere above.
> +/**
> + * Moves a submodule at a given path from a given head to another new head.
> + * For edge cases (a submodule coming into existence or removing a submodule)
> + * pass NULL for old or new respectively.
> + *
> + * TODO: move dryrun and forced to flags.
The reason why this seeingly trivial thing is left as TODO is...???
> + */
> +int submodule_go_from_to(const char *path,
> + const char *old,
> + const char *new,
> + int dry_run,
> + int force)
> +{
go-from-to does not tell me what it does, but my cursory read of the
body of the function tells me that this is doing a checkout of a
branch in the submodule? The operation in builtin/checkout.c that
conceptually correspond to this is called switch_branches(), I
think, so perhaps submodule_switch_branches() is a better name?
> + int ret = 0;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + const struct submodule *sub;
> +
> + sub = submodule_from_path(null_sha1, path);
> +
> + if (!sub)
> + die("BUG: could not get submodule information for '%s'", path);
> +
> + if (!dry_run) {
> + if (old) {
> + if (!submodule_uses_gitfile(path))
> + absorb_git_dir_into_superproject("", path,
> + ABSORB_GITDIR_RECURSE_SUBMODULES);
> + } else {
> + struct strbuf sb = STRBUF_INIT;
> + strbuf_addf(&sb, "%s/modules/%s",
> + get_git_common_dir(), sub->name);
> + connect_work_tree_and_git_dir(path, sb.buf);
> + strbuf_release(&sb);
> +
> + /* make sure the index is clean as well */
> + submodule_clean_index(path);
> + }
> + }
> +
> + if (old && !force) {
> + /* Check if the submodule has a dirty index. */
> + if (submodule_has_dirty_index(sub)) {
> + /* print a thing here? */
> + return -1;
> + }
Isn't it too late to do this here? You already reset the index
in the submodule, no?
Is the idea that changes that are only in the submodule's working
tree are noticed by later "read-tree -u -m" down there? Not
complaining but trying to understand.
> + }
> +
> + prepare_submodule_repo_env_no_git_dir(&cp.env_array);
> +
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> +
> + argv_array_pushf(&cp.args, "--super-prefix=%s/", path);
> + argv_array_pushl(&cp.args, "read-tree", NULL);
> +
> + if (dry_run)
> + argv_array_push(&cp.args, "-n");
> + else
> + argv_array_push(&cp.args, "-u");
> +
> + if (force)
> + argv_array_push(&cp.args, "--reset");
> + else
> + argv_array_push(&cp.args, "-m");
> +
> + argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);
> + argv_array_push(&cp.args, new ? new : EMPTY_TREE_SHA1_HEX);
> +
> + if (run_command(&cp)) {
> + ret = -1;
> + goto out;
> + }
> +
> + if (!dry_run) {
> + if (new) {
> + struct child_process cp1 = CHILD_PROCESS_INIT;
> + /* also set the HEAD accordingly */
> + cp1.git_cmd = 1;
> + cp1.no_stdin = 1;
> + cp1.dir = path;
> +
> + argv_array_pushl(&cp1.args, "update-ref", "HEAD",
> + new ? new : EMPTY_TREE_SHA1_HEX, NULL);
> +
> + if (run_command(&cp1)) {
> + ret = -1;
> + goto out;
> ...
^ permalink raw reply
* Re: [PATCH 08/15] submodules: introduce check to see whether to touch a submodule
From: Junio C Hamano @ 2017-02-16 21:02 UTC (permalink / raw)
To: Stefan Beller
In-Reply-To: <20170216003811.18273-9-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> In later patches we introduce the --recurse-submodule flag for commands
> that modify the working directory, e.g. git-checkout.
>
> It is potentially expensive to check if a submodule needs an update,
> because a common theme to interact with submodules is to spawn a child
> process for each interaction.
>
> So let's introduce a function that checks if a submodule needs
> to be checked for an update before attempting the update.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> submodule.c | 27 +++++++++++++++++++++++++++
> submodule.h | 13 +++++++++++++
> 2 files changed, 40 insertions(+)
>
> diff --git a/submodule.c b/submodule.c
> index 591f4a694e..2a37e03420 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -548,6 +548,33 @@ void set_config_update_recurse_submodules(int value)
> config_update_recurse_submodules = value;
> }
>
> +int touch_submodules_in_worktree(void)
> +{
> + /*
> + * Update can't be "none", "merge" or "rebase",
> + * treat any value as OFF, except an explicit ON.
> + */
> + return config_update_recurse_submodules == RECURSE_SUBMODULES_ON;
> +}
I somehow sense a somewhat misnamed function.
> +int is_active_submodule_with_strategy(const struct cache_entry *ce,
> + enum submodule_update_type strategy)
> +{
> + const struct submodule *sub;
> +
> + if (!S_ISGITLINK(ce->ce_mode))
> + return 0;
> +
> + if (!touch_submodules_in_worktree())
> + return 0;
Reading this caller alone, it is totally unclear what this !touch is
about. "We try to touch it by calling this function, and if the
function successfullly touches it, we return without doing anything
else?"
Would it help to avoid confusion, if the helper function is named to
be clearly a boolean? should_update_submodules_in_worktree() or
something along those lines?
> + sub = submodule_from_path(null_sha1, ce->name);
> + if (!sub)
> + return 0;
> +
> + return sub->update_strategy.type == strategy;
> +}
I am not sure if this is a good API design; if it were "static int"
contained inside the module I wouldn't care, but wouldn't it be more
natural for the caller of this function to say
if (get_submodule_update_strategy(ce) == STRATEGY_I_WANT)
do something;
else
do something else;
rather than forced to say:
if (is_active_submodule_with_strategy(ce, STRATEGY_I_WANT))
do something;
else
do something else;
no? The caller can easily be extended to
switch (get_submodule_update_strategy(ce)) {
case STRATEGY_I_WANT:
case STRATEGY_I_TOLERATE:
do something;
break;
default:
do something else;
break;
}
if the function does not insist taking a single allowed strategy and
return just yes/no as its answer.
^ permalink raw reply
* Re: Back quote typo in error messages (?)
From: Fabrizio Cucci @ 2017-02-16 20:58 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170216190200.h362e52uxqlkduar@sigill.intra.peff.net>
On 16 February 2017 at 19:02, Jeff King <peff@peff.net> wrote:
> Try the "Quoted Text" section of the original asciidoc user manual:
>
> http://www.methods.co.nz/asciidoc/userguide.html#X51
>
> Asciidoctor has introduced some new syntax (almost certainly because the
> original asymmetric formulations have a bunch of ambiguous corner
> cases). By default, it disables the asymmetric versions, but they work
> in "compat" mode (and the newer ones do not).
I can't say I had the pleasure of using Asciidoctor 0.1.4 or earlier! :)
> Git's documentation is all written for the original asciidoc. If you
> build it with asciidoctor, it must be done in compat mode. This is the
> default when asciidoctor sees a two-line (i.e., underlined) section
> title, which all of our manpages have.
And I definitely didn't know that, but now I'm glad we went OT! :)
Thanks a lot for the clarifications.
^ permalink raw reply
* Re: [PATCH 05/15] connect_work_tree_and_git_dir: safely create leading directories
From: Junio C Hamano @ 2017-02-16 20:54 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, sandals, jrnieder, bmwill
In-Reply-To: <20170216003811.18273-6-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> In a later patch we'll use connect_work_tree_and_git_dir when the
> directory for the gitlink file doesn't exist yet. Safely create
> the directory first.
>
> One of the two users of 'connect_work_tree_and_git_dir' already checked
> for the directory being there, so we can loose that check.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> dir.c | 32 +++++++++++++++++++++-----------
> submodule.c | 11 ++---------
> 2 files changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 4541f9e146..6f52af7abb 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2728,23 +2728,33 @@ void untracked_cache_add_to_index(struct index_state *istate,
> /* Update gitfile and core.worktree setting to connect work tree and git dir */
> void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
> {
> - struct strbuf file_name = STRBUF_INIT;
> + struct strbuf gitfile_sb = STRBUF_INIT;
> + struct strbuf cfg_sb = STRBUF_INIT;
> struct strbuf rel_path = STRBUF_INIT;
> - char *git_dir = real_pathdup(git_dir_);
> - char *work_tree = real_pathdup(work_tree_);
> + char *git_dir, *work_tree;
>
> - /* Update gitfile */
> - strbuf_addf(&file_name, "%s/.git", work_tree);
> - write_file(file_name.buf, "gitdir: %s",
> - relative_path(git_dir, work_tree, &rel_path));
> + /* Prepare .git file */
> + strbuf_addf(&gitfile_sb, "%s/.git", work_tree_);
> + if (safe_create_leading_directories_const(gitfile_sb.buf))
> + die(_("could not create directories for %s"), gitfile_sb.buf);
> +
> + /* Prepare config file */
> + strbuf_addf(&cfg_sb, "%s/config", git_dir_);
> + if (safe_create_leading_directories_const(cfg_sb.buf))
> + die(_("could not create directories for %s"), cfg_sb.buf);
>
> + git_dir = real_pathdup(git_dir_);
> + work_tree = real_pathdup(work_tree_);
> +
> + /* Write .git file */
> + write_file(gitfile_sb.buf, "gitdir: %s",
> + relative_path(git_dir, work_tree, &rel_path));
The above does somewhat more than advertised and was a bit hard to
grok. Initially I thought the reason why pathdup()s were delayed
was perhaps because you pathdup() something potentially different
from the given parameter to the function (i.e. new code before
pathdup() may tweak what is pathdup()ed).
But that is not what is happening. I suspect that you did this to
avoid leaking allocated memory when the code calls die().
If the code was written like so from the beginning, I do not see a
reason to move the pathdup() up to deliberately make it leak [*1*].
But as a part of this change, I found it distracting and getting in
the way of understanding the change. If you really care, it is
nicer to do it to reviewers as a separate preparatory clean-up step,
or follow-up standalone clean-up patch after the series settles.
The comment "prepare config file" was misleading; it is preparing
the place the config file will be created (same for "prepare .git
file").
It is a good thing to do to make sure git_dir_ exists and it should
be mentioned in the log message. Adding "Do the same for the place
where the per-repo config file is created". at the end of the first
paragraph should be sufficient.
Thanks.
[Footnote]
*1* it is arguable a small piece of unfreed memory before exit() or
die() is worth called "leak", though.
^ permalink raw reply
* Re: [PATCH 03/15] lib-submodule-update.sh: define tests for recursing into submodules
From: Junio C Hamano @ 2017-02-16 20:39 UTC (permalink / raw)
To: Stefan Beller
In-Reply-To: <20170216003811.18273-4-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Currently lib-submodule-update.sh provides 2 functions
> test_submodule_switch and test_submodule_forced_switch that are used by a
> variety of tests to ensure that submodules behave as expected. The current
> expected behavior is that submodules are not touched at all (see
> 42639d2317a for the exact setup).
>
> In the future we want to teach all these commands to properly recurse
> into submodules. To do that, we'll add two testing functions to
> submodule-update-lib.sh test_submodule_switch_recursing and
> test_submodule_forced_switch_recursing.
I'd remove "properly" and insert a colon after "add two ... to
submodule-update-lib.sh" before the names of two functions.
> +reset_work_tree_to_interested () {
> + reset_work_tree_to $1 &&
> + # indicate we are interested in the submodule:
> + git -C submodule_update config submodule.sub1.url "bogus" &&
> + # also have it available:
> + if ! test -d submodule_update/.git/modules/sub1
> + then
> + mkdir submodule_update/.git/modules &&
Would we want "mkdir -p" here to be safe?
> + cp -r submodule_update_repo/.git/modules/sub1 submodule_update/.git/modules/sub1
... ahh, wouldn't matter that much, we checked that modules/sub1
does not exist, and as long as nobody creates modules/ or modules/somethingelse
we are OK.
> + fi
> +}
> +
> @@ -695,3 +736,443 @@ test_submodule_forced_switch () {
> )
> '
> }
> +
> +# Test that submodule contents are correctly updated when switching
> +# between commits that change a submodule.
> +# Test that the following transitions are correctly handled:
> +# (These tests are also above in the case where we expect no change
> +# in the submodule)
> +# - Updated submodule
> +# - New submodule
> +# - Removed submodule
> +# - Directory containing tracked files replaced by submodule
> +# - Submodule replaced by tracked files in directory
> +# - Submodule replaced by tracked file with the same name
> +# - tracked file replaced by submodule
These should work without trouble only when the paths involved in
the operation in the working tree are clean, right? Just double
checking. If they are dirty we should expect a failure, instead of
silent loss of information.
> +# New test cases
> +# - Removing a submodule with a git directory absorbs the submodules
> +# git directory first into the superproject.
> +
> +test_submodule_switch_recursing () {
> + command="$1"
The dq-pair is not strictly needed on the RHS of the assignment, but
it is a good way to signal that we considered that we might receive
an argument with $IFS in it...
> + ######################### Appearing submodule #########################
> + # Switching to a commit letting a submodule appear checks it out ...
> + test_expect_success "$command: added submodule is checked out" '
> + prolog &&
> + reset_work_tree_to_interested no_submodule &&
> + (
> + cd submodule_update &&
> + git branch -t add_sub1 origin/add_sub1 &&
> + $command add_sub1 &&
... and after doing so, not quoting $command here signals that we
expect command line splitting to happen. Am I reading it correctly?
Without an actual caller appearing in this step, it is rather hard
to judge.
> + test_superproject_content origin/add_sub1 &&
> + test_submodule_content sub1 origin/add_sub1
> + )
> ...
> + # ... but an ignored file is fine.
> + test_expect_success "$command: added submodule removes an untracked ignored file" '
> + test_when_finished "rm submodule_update/.git/info/exclude" &&
> + prolog &&
> + reset_work_tree_to_interested no_submodule &&
> + (
> + cd submodule_update &&
> + git branch -t add_sub1 origin/add_sub1 &&
> + : >sub1 &&
> + echo sub1 > .git/info/exclude
">.git/info/exclude"
> + $command add_sub1 &&
> + test_superproject_content origin/add_sub1 &&
> + test_submodule_content sub1 origin/add_sub1
> + )
> + '
^ permalink raw reply
* Re: [PATCH v3] reset: add an example of how to split a commit into two
From: Junio C Hamano @ 2017-02-16 19:49 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Jacob Keller
In-Reply-To: <xmqq4lzux7sc.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> The interdiff between v2 and v3 is not really worth showing since I
>> basically re-wrote the entire section a bit.
>
> Could this be made into an incremental, now that v2 has been in
> 'next' for about 10 days, please?
Nah, I think it is easier to read "log -p" if I just revert v2 out
of existence from 'next', and queue this (with a minor typofixes) as
a different topic to be merged later to 'master'.
So no need to resend and certainly no need to make it incremental.
>> +Split a commit apart into a sequence of commits::
>> ++
>> +Suppose that you have create lots of logically separate changes and commit them
>
> s/create/&d/; s/commit/&ed/
I'd do this myself while queuing.
Thanks.
>
>> +together. Then, later you decide that it might be better to have each logical
>> +chunk associated with its own commit. ...
^ permalink raw reply
* Re: [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"
From: Siddharth Kannan @ 2017-02-16 19:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Git List, Pranit Bauva, Jeff King, pclouds,
brian m. carlson
In-Reply-To: <xmqqinoax96u.fsf@gitster.mtv.corp.google.com>
Hey Junio and Matthieu,
On 17 February 2017 at 00:19, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>>
>>> This is as per our discussion[1]. The patches and commit messages are based on
>>> Junio's patches that were posted as a reply to
>>> <20170212184132.12375-1-gitster@pobox.com>.
>>>
>>> As per Matthieu's comments, I have updated the tests, but there is still one
>>> thing that is not working: log -@{yesterday} or log -@{2.days.ago}
>>
>> Note that I did not request that these things work, just that they seem
>> to be relevant tests: IMHO it's OK to reject them, but for example we
>> don't want them to segfault. And having a test is a good hint that you
>> thought about what could happen and to document it.
>
> The branch we were on before would be a ref, and the ref may know
> where it was yesterday? If @{-1}@{1.day} works it would be natural
> to expect -@{1.day} to, too, but there probably is some disambiguity
> or other reasons that they cannot or should not work that way I am
> missing, in which case it is fine ("too much work for too obscure
> feature that is not expected to be used often" is also an acceptable
> reason) to punt or deliberately not support it, as long as it is
> explained in the log and/or doc (future developers need to know if
> we are simply punting, or if we found a case where it would hurt end
> user experience if we supported the feature), and as long as it does
> not do a wrong thing (dying with "we do not support it" is OK,
> segfaulting or doing random other things is not).
>
Right now, these commands die with an "fatal: unrecognized argument:
-@{yesterday}" or a "fatal: unrecognized argument: -@{2.days.ago}".
So, it is definitely not doing anything "random" :)
I will wait for consensus on whether these should or should not be
supported.
--
Best Regards,
- Siddharth.
^ permalink raw reply
* Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option
From: Siddharth Kannan @ 2017-02-16 19:39 UTC (permalink / raw)
To: Matthieu Moy
Cc: Junio C Hamano, Git List, Pranit Bauva, Jeff King, pclouds,
brian m. carlson
In-Reply-To: <vpqwpcqgfmw.fsf@anie.imag.fr>
Hey Matthieu,
On 16 February 2017 at 23:52, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>
> Indeed, I misread the patch. The explanation could be a little bit more
> "tired-reviewer-proof" by not using a past tone, perhaps
>
> 1. setup_revision, which is changed to ...
Oh, okay! Sorry about the confusion!
Yes, I used the past perfect tense to refer to changes that were made
in this particular patch!
I will change the message in the next version to something that's in
present tense.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
--
Best Regards,
- Siddharth.
^ permalink raw reply
* Re: [PATCH 1/1] config.txt: Fix formatting of submodule.alternateErrorStrategy section
From: Junio C Hamano @ 2017-02-16 19:23 UTC (permalink / raw)
To: Stefan Beller; +Cc: David Pursehouse, git@vger.kernel.org, David Pursehouse
In-Reply-To: <CAGZ79kaPFP3aQB8=gZ+BvRUqJa+NPuDQ+5kvKNqqYs3S28EEew@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Wed, Feb 15, 2017 at 9:05 PM, David Pursehouse
> <david.pursehouse@gmail.com> wrote:
>> From: David Pursehouse <dpursehouse@collab.net>
>>
>> Add missing `::` after the title.
>>
>> Signed-off-by: David Pursehouse <dpursehouse@collab.net>
>
> Thanks!
> Stefan
Thanks, both.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox