* [PATCH v2 10/34] sequencer (rebase -i): allow continuing with staged changes
From: Johannes Schindelin @ 2016-12-13 15:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <cover.1481642927.git.johannes.schindelin@gmx.de>
When an interactive rebase is interrupted, the user may stage changes
before continuing, and we need to commit those changes in that case.
Please note that the nested "if" added to the sequencer_continue() is
not combined into a single "if" because it will be extended with an
"else" clause in a later patch in this patch series.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 80469b6954..855d3ba503 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1829,6 +1829,42 @@ static int continue_single_pick(void)
return run_command_v_opt(argv, RUN_GIT_CMD);
}
+static int commit_staged_changes(struct replay_opts *opts)
+{
+ int amend = 0;
+
+ if (has_unstaged_changes(1))
+ return error(_("cannot rebase: You have unstaged changes."));
+ if (!has_uncommitted_changes(0))
+ return 0;
+
+ if (file_exists(rebase_path_amend())) {
+ struct strbuf rev = STRBUF_INIT;
+ unsigned char head[20], to_amend[20];
+
+ if (get_sha1("HEAD", head))
+ return error(_("cannot amend non-existing commit"));
+ if (!read_oneliner(&rev, rebase_path_amend(), 0))
+ return error(_("invalid file: '%s'"), rebase_path_amend());
+ if (get_sha1_hex(rev.buf, to_amend))
+ return error(_("invalid contents: '%s'"),
+ rebase_path_amend());
+ if (hashcmp(head, to_amend))
+ return error(_("\nYou have uncommitted changes in your "
+ "working tree. Please, commit them\n"
+ "first and then run 'git rebase "
+ "--continue' again."));
+
+ strbuf_release(&rev);
+ amend = 1;
+ }
+
+ if (run_git_commit(rebase_path_message(), opts, 1, 1, amend, 0))
+ return error(_("could not commit staged changes."));
+ unlink(rebase_path_amend());
+ return 0;
+}
+
int sequencer_continue(struct replay_opts *opts)
{
struct todo_list todo_list = TODO_LIST_INIT;
@@ -1837,6 +1873,10 @@ int sequencer_continue(struct replay_opts *opts)
if (read_and_refresh_cache(opts))
return -1;
+ if (is_rebase_i(opts)) {
+ if (commit_staged_changes(opts))
+ return -1;
+ }
if (!file_exists(get_todo_path(opts)))
return continue_single_pick();
if (read_populate_opts(opts))
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v2 11/34] sequencer (rebase -i): remove CHERRY_PICK_HEAD when no longer needed
From: Johannes Schindelin @ 2016-12-13 15:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <cover.1481642927.git.johannes.schindelin@gmx.de>
The scripted version of the interactive rebase already does that.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 855d3ba503..abffaf3b40 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1835,8 +1835,13 @@ static int commit_staged_changes(struct replay_opts *opts)
if (has_unstaged_changes(1))
return error(_("cannot rebase: You have unstaged changes."));
- if (!has_uncommitted_changes(0))
+ if (!has_uncommitted_changes(0)) {
+ const char *cherry_pick_head = git_path("CHERRY_PICK_HEAD");
+
+ if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
+ return error(_("could not remove CHERRY_PICK_HEAD"));
return 0;
+ }
if (file_exists(rebase_path_amend())) {
struct strbuf rev = STRBUF_INIT;
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v2 30/34] sequencer (rebase -i): show only failed cherry-picks' output
From: Johannes Schindelin @ 2016-12-13 15:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <cover.1481642927.git.johannes.schindelin@gmx.de>
This is the behavior of the shell script version of the interactive
rebase, by using the `output` function defined in `git-rebase.sh`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index dfa4fab98b..edc213a2c8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -464,6 +464,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
o.ancestor = base ? base_label : "(empty tree)";
o.branch1 = "HEAD";
o.branch2 = next ? next_label : "(empty tree)";
+ if (is_rebase_i(opts))
+ o.buffer_output = 2;
head_tree = parse_tree_indirect(head);
next_tree = next ? next->tree : empty_tree();
@@ -475,6 +477,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
clean = merge_trees(&o,
head_tree,
next_tree, base_tree, &result);
+ if (is_rebase_i(opts) && clean <= 0)
+ fputs(o.obuf.buf, stdout);
strbuf_release(&o.obuf);
if (clean < 0)
return clean;
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v2 29/34] sequencer (rebase -i): show only failed `git commit`'s output
From: Johannes Schindelin @ 2016-12-13 15:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <cover.1481642927.git.johannes.schindelin@gmx.de>
This is the behavior of the shell script version of the interactive
rebase, by using the `output` function defined in `git-rebase.sh`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 63f6f25ced..dfa4fab98b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -647,10 +647,15 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
{
char **env = NULL;
struct argv_array array;
- int rc;
+ int opt = RUN_GIT_CMD, rc;
const char *value;
if (is_rebase_i(opts)) {
+ if (!edit) {
+ opt |= RUN_COMMAND_STDOUT_TO_STDERR;
+ opt |= RUN_HIDE_STDERR_ON_SUCCESS;
+ }
+
env = read_author_script();
if (!env) {
const char *gpg_opt = gpg_sign_opt_quoted(opts);
@@ -687,7 +692,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
if (opts->allow_empty_message)
argv_array_push(&array, "--allow-empty-message");
- rc = run_command_v_opt_cd_env(array.argv, RUN_GIT_CMD, NULL,
+ rc = run_command_v_opt_cd_env(array.argv, opt, NULL,
(const char *const *)env);
argv_array_clear(&array);
free(env);
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
From: Johannes Schindelin @ 2016-12-13 15:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqoa0g96o3.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 12 Dec 2016, Junio C Hamano wrote:
> * js/mingw-isatty (2016-12-11) 1 commit
> (merged to 'next' on 2016-12-12 at 60c1da6676)
> + mingw: intercept isatty() to handle /dev/null as Git expects it
>
> We often decide if a session is interactive by checking if the
> standard I/O streams are connected to a TTY, but isatty() emulation
> on Windows incorrectly returned true if it is used on NUL (i.e. an
> equivalent to /dev/null). This has been fixed.
I'd like to suggest a reword: we did not use an isatty() emulation, but
Windows' own _isatty() function that simply has different semantics than
what Git expected. *Now* we have an isatty() emulation that wraps
_isatty() and emulates the behavior expected by Git.
Thanks,
Dscho
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
From: Johannes Schindelin @ 2016-12-13 15:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqoa0g96o3.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 12 Dec 2016, Junio C Hamano wrote:
> * jc/bundle (2016-03-03) 6 commits
> - index-pack: --clone-bundle option
> - Merge branch 'jc/index-pack' into jc/bundle
> - bundle v3: the beginning
> - bundle: keep a copy of bundle file name in the in-core bundle header
> - bundle: plug resource leak
> - bundle doc: 'verify' is not about verifying the bundle
>
> The beginning of "split bundle", which could be one of the
> ingredients to allow "git clone" traffic off of the core server
> network to CDN.
>
> While I think it would make it easier for people to experiment and
> build on if the topic is merged to 'next', I am at the same time a
> bit reluctant to merge an unproven new topic that introduces a new
> file format, which we may end up having to support til the end of
> time. It is likely that to support a "prime clone from CDN", it
> would need a lot more than just "these are the heads and the pack
> data is over there", so this may not be sufficient.
>
> Will discard.
You could mark it as experimental, subject to change, and merge it to
`next` safely.
Ciao,
Dscho
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
From: Junio C Hamano @ 2016-12-13 16:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1612131638290.23160@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Junio,
>
> On Mon, 12 Dec 2016, Junio C Hamano wrote:
>
>> * js/mingw-isatty (2016-12-11) 1 commit
>> (merged to 'next' on 2016-12-12 at 60c1da6676)
>> + mingw: intercept isatty() to handle /dev/null as Git expects it
>>
>> We often decide if a session is interactive by checking if the
>> standard I/O streams are connected to a TTY, but isatty() emulation
>> on Windows incorrectly returned true if it is used on NUL (i.e. an
>> equivalent to /dev/null). This has been fixed.
>
> I'd like to suggest a reword: we did not use an isatty() emulation, but
> Windows' own _isatty() function that simply has different semantics than
> what Git expected. *Now* we have an isatty() emulation that wraps
> _isatty() and emulates the behavior expected by Git.
Thanks for a comment.
One of the things that the new code does with the fix is this:
+/* In this file, we actually want to use Windows' own isatty(). */
+#undef isatty
+
which undoes "#define isatty winansi_isatty" that other code uses,
so that the implementation of winansi_isatty() can say isatty() and
get what people usually get when they say "isatty()" on Windows.
Before or after that patch, there is no "#define isatty _isatty" in
our codebase. I take all of the above to mean that Windows does
give us isatty() function (not a macro--as otherwise it won't become
available to us again by "#undef isatty"), that in turn internally
calls what it calls _isatty() that says true for NUL?
Following the above reasoning, I meant "whatever you get when you
write isatty() on Windows" by "isatty() emulation on Windows" in the
paragraph you are commenting on. I didn't say "what was written by
Git for Windows folks to emulate isatty()" or "what was given by MS
development tools", as that distinction is immaterial and does not
change the value of the fix.
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
From: Junio C Hamano @ 2016-12-13 16:13 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1612131641291.23160@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> While I think it would make it easier for people to experiment and
>> build on if the topic is merged to 'next', I am at the same time a
>> bit reluctant to merge an unproven new topic that introduces a new
>> file format, which we may end up having to support til the end of
>> time. It is likely that to support a "prime clone from CDN", it
>> would need a lot more than just "these are the heads and the pack
>> data is over there", so this may not be sufficient.
>>
>> Will discard.
>
> You could mark it as experimental, subject to change, and merge it to
> `next` safely.
Are you planning, or do you know somebody who plans to use that code
soonish? Otherwise I'd prefer to drop it---at this point, the series
is merely "just because we can", not "because we need it to further
improve this or that".
^ permalink raw reply
* Re: [RFC/PATCH v3 00/16] Add initial experimental external ODB support
From: Christian Couder @ 2016-12-13 16:40 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey, Lars Schneider,
Eric Wong, Christian Couder
In-Reply-To: <xmqqy400bno3.fsf@gitster.mtv.corp.google.com>
On Wed, Nov 30, 2016 at 11:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> For now there should be one odb ref per blob. Each ref name should be
>> refs/odbs/<odbname>/<sha1> where <sha1> is the sha1 of the blob stored
>> in the external odb named <odbname>.
>>
>> These odb refs should all point to a blob that should be stored in the
>> Git repository and contain information about the blob stored in the
>> external odb. This information can be specific to the external odb.
>> The repos can then share this information using commands like:
>>
>> `git fetch origin "refs/odbs/<odbname>/*:refs/odbs/<odbname>/*"`
>
> Unless this is designed to serve only a handful of blobs, I cannot
> see how this design would scale successfully. I notice you wrote
> "For now" at the beginning, but what is the envisioned way this will
> evolve in the future?
In general I think that having a lot of refs is really a big problem
right now in Git as many big organizations using Git are facing this
problem in one form or another.
So I think that support for a big number of refs is a separate and
important problem that should and hopefully will be solved.
My preferred way to solve it would be with something like Shawn's
RefTree. I think it would also help regarding other problems like
speeding up git protocol, tracking patch series (see git-series
discussions), tools like https://www.softwareheritage.org/, ...
If the "big number of refs" problem is not solved and many refs in
refs/odbs/<odbname>/ is a problem, it's possible to have just one ref
in refs/odbs/<odbname>/ that points to a blob that contains a list
(maybe a json list with information attached to each item) of the
blobs stored in the external odb. Though I think it would be more
complex to edit/parse this list than to deal with many refs in
refs/odbs/<odbname>/.
^ permalink raw reply
* Re: [RFC/PATCH v3 00/16] Add initial experimental external ODB support
From: Christian Couder @ 2016-12-13 17:20 UTC (permalink / raw)
To: Lars Schneider
Cc: git, Junio C Hamano, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey,
Eric Wong, Christian Couder
In-Reply-To: <A5ABBF3E-BED9-4FF3-9DE5-B529DEF0B8E8@gmail.com>
On Sat, Dec 3, 2016 at 7:47 PM, Lars Schneider <larsxschneider@gmail.com> wrote:
>
>> On 30 Nov 2016, at 22:04, Christian Couder <christian.couder@gmail.com> wrote:
>>
>> Goal
>> ~~~~
>>
>> Git can store its objects only in the form of loose objects in
>> separate files or packed objects in a pack file.
>>
>> To be able to better handle some kind of objects, for example big
>> blobs, it would be nice if Git could store its objects in other object
>> databases (ODB).
>
> This is a great goal. I really hope we can use that to solve the
> pain points in the current Git <--> GitLFS integration!
Yeah, I hope it will help too.
> Thanks for working on this!
>
> Minor nit: I feel the term "other" could be more expressive. Plus
> "database" might confuse people. What do you think about
> "External Object Storage" or something?
In the current Git code, "DB" is already used a lot. For example in
cache.h there is:
#define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
#define INIT_DB_QUIET 0x0001
#define INIT_DB_EXIST_OK 0x0002
extern int init_db(const char *git_dir, const char *real_git_dir,
const char *template_dir, unsigned int flags);
[...]
>> - "<command> get <sha1>": the command should then read from the
>> external ODB the content of the object corresponding to <sha1> and
>> output it on stdout.
>>
>> - "<command> put <sha1> <size> <type>": the command should then read
>> from stdin an object and store it in the external ODB.
>
> Based on my experience with Git clean/smudge filters I think this kind
> of single shot protocol will be a performance bottleneck as soon as
> people store more than >1000 files in the external ODB.
> Maybe you can reuse my "filter process protocol" (edcc858) here?
Yeah, I would like to do reuse your "filter process protocol" as much
as possible to improve this in the future.
>> * Transfer
>>
>> To tranfer information about the blobs stored in external ODB, some
>> special refs, called "odb ref", similar as replace refs, are used.
>>
>> For now there should be one odb ref per blob. Each ref name should be
>> refs/odbs/<odbname>/<sha1> where <sha1> is the sha1 of the blob stored
>> in the external odb named <odbname>.
>>
>> These odb refs should all point to a blob that should be stored in the
>> Git repository and contain information about the blob stored in the
>> external odb. This information can be specific to the external odb.
>> The repos can then share this information using commands like:
>>
>> `git fetch origin "refs/odbs/<odbname>/*:refs/odbs/<odbname>/*"`
>
> The "odbref" would point to a blob and the blob could contain anything,
> right? E.g. it could contain an existing GitLFS pointer, right?
>
> version https://git-lfs.github.com/spec/v1
> oid sha256:4d7a214614ab2935c943f9e0ff69d22eadbb8f32b1258daaa5e2ca24d17e2393
> size 12345
Yes, but I think that the sha1 should be added. So yes, it could
easily be made compatible with git LFS.
>> Design discussion about performance
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Yeah, it is not efficient to fork/exec a command to just read or write
>> one object to or from the external ODB. Batch calls and/or using a
>> daemon and/or RPC should be used instead to be able to store regular
>> objects in an external ODB. But for now the external ODB would be all
>> about really big files, where the cost of a fork+exec should not
>> matter much. If we later want to extend usage of external ODBs, yeah
>> we will probably need to design other mechanisms.
>
> I think we should leverage the learnings from GitLFS as much as possible.
> My learnings are:
>
> (1) Fork/exec per object won't work. People have lots and lots of content
> that is not suited for Git (e.g. integration test data, images, ...).
I agree that it will not work for many people, but look at how git LFS
evolved. It first started without a good solution for those people,
and then you provided a much better solution to them.
So I am a bit reluctant to work on a complex solution reusing your
"filter process protocol" work right away.
> (2) We need a good UI. I think it would be great if the average user would
> not even need to know about ODB. Moving files explicitly with a "put"
> command seems unpractical to me. GitLFS tracks files via filename and
> that has a number of drawbacks, too. Do you see a way to define a
> customizable metric such as "move all files to ODB X that are gzip
> compressed larger than Y"?
I think these should be defined in the config and attributes files. It
could also be possible to implement a "want" command (in the same way
as the "get", "put" and "have" commands) to ask the e-odb helper if it
wants to store a specific blob.
>> Future work
>> ~~~~~~~~~~~
>>
>> I think that the odb refs don't prevent a regular fetch or push from
>> wanting to send the objects that are managed by an external odb. So I
>> am interested in suggestions about this problem. I will take a look at
>> previous discussions and how other mechanisms (shallow clone, bundle
>> v3, ...) handle this.
>
> If the ODB configuration is stored in the Git repo similar to
> .gitmodules then every client that clones ODB references would be able
> to resolve them, right?
Yeah, but I am not sure that being able to resolve the odb refs will
prevent the big blobs from being sent.
With Git LFS, git doesn't know about the big blobs, only about the
substituted files, but that is not the case in what I am doing.
Thanks,
Christian.
^ permalink raw reply
* Re: git add -p with new file
From: Jeff King @ 2016-12-13 17:33 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Junio C Hamano, Ariel, git
In-Reply-To: <dc698b79-6311-a2a3-c564-a43ef071e62b@gmx.net>
On Mon, Dec 12, 2016 at 09:31:03PM +0100, Stephan Beyer wrote:
> I am also a "git add -p"-only user (except for new files and merges).
> However, I usually keep a lot of untracked files in my repositories.
> Files that I do not (git)ignore because I want to see them when I type
> "git status".
>
> Hence, the imagination only that "git add -p" starts to ask me for each
> untracked file feels like a lot of annoying "n" presses. I could imagine
> that it is okay-ish when it asks about the untracked files *after* all
> tracked paths have been processed (I guess this has been proposed
> before), so that I can safely quit.
Yeah, this is the "some people might be annoyed" that I mentioned
originally. If your workflow leaves a lot of untracked files that you
don't care about it, then I think you'd want this feature disabled
entirely via a config option (or vice versa, that it would only be
enabled by config option).
> I am also not really sure what problem this feature is trying to solve.
> If the "problem"(?) is that it should act more like "git add" instead of
> "git add -u", for whatever reason, this may be fine (but the
> configuration option is a must-have then).
I think the problem is just that "add -p" does not give the whole story
of what you might want to do before making a commit.
> > I'd also probably add interactive.showUntracked to make the whole thing
> > optional (but I think it would be OK to default it to on).
> Hm, "interactive.showUntracked" is a confusing name because "git add -i"
> (interactive) already handles untracked files.
Sure, that was just meant for illustration. I agree there's probably a
better name.
-Peff
^ permalink raw reply
* Re: [PATCH 6/6] rm: add absorb a submodules git dir before deletion
From: Stefan Beller @ 2016-12-13 17:51 UTC (permalink / raw)
To: brian m. carlson, Stefan Beller, Junio C Hamano,
git@vger.kernel.org, David Turner, Brandon Williams
In-Reply-To: <20161213032848.4ps42jinix6fdgdc@genre.crustytoothpaste.net>
On Mon, Dec 12, 2016 at 7:28 PM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On Mon, Dec 12, 2016 at 05:40:55PM -0800, Stefan Beller wrote:
>> When deleting a submodule we need to keep the actual git directory around,
>> such that we do not lose local changes in there and at a later checkout
>> of the submodule we don't need to clone it again.
>>
>> Implement `depopulate_submodule`, that migrates the git directory before
>> deletion of a submodule and afterwards the equivalent of "rm -rf", which
>> is already found in entry.c, so expose that and for clarity add a suffix
>> "_or_dir" to it.
>
> I think you might have meant "_or_die" here.
indeed, will fix in a reroll. Thanks for the review!
^ permalink raw reply
* Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
From: Stefan Beller @ 2016-12-13 17:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, David Turner, Brandon Williams
In-Reply-To: <xmqqr35c5luq.fsf@gitster.mtv.corp.google.com>
On Mon, Dec 12, 2016 at 11:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The "checkout --recurse-submodules" series got too large to comfortably send
>> it out for review, so I had to break it up into smaller series'; this is the
>> first subseries, but it makes sense on its own.
>>
>> This series teaches git-rm to absorb the git directory of a submodule instead
>> of failing and complaining about the git directory preventing deletion.
>>
>> It applies on origin/sb/submodule-embed-gitdir.
>
> Thanks. I probably should rename the topic again with s/embed/absorb/;
I mostly renamed it in the hope to settle our dispute what embedding means. ;)
So in case we want to further discuss on the name of the function, we should
do that before doing actual work such as renaming.
Note that sb/t3600-cleanup is part of this series now,
(The first commit of that series is in patch 6/6 of this series, and patch 5 is
the modernization effort.)
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCHv2 1/2] merge: Add '--continue' option as a synonym for 'git commit'
From: Junio C Hamano @ 2016-12-13 18:02 UTC (permalink / raw)
To: Chris Packham; +Cc: git, mah, peff, jacob.keller
In-Reply-To: <20161213084859.13426-1-judge.packham@gmail.com>
Chris Packham <judge.packham@gmail.com> writes:
> +'git merge' --continue
>
> DESCRIPTION
> -----------
> @@ -61,6 +62,9 @@ reconstruct the original (pre-merge) changes. Therefore:
> discouraged: while possible, it may leave you in a state that is hard to
> back out of in the case of a conflict.
>
> +The fourth syntax ("`git merge --continue`") can only be run after the
> +merge has resulted in conflicts.
OK. I can see that the code refuses if there is no MERGE_HEAD, so
"can only be run" is ensured correctly.
> 'git merge --continue' will take the
> +currently staged changes and complete the merge.
For Git-savvy folks, this may be sufficient to tell that they are
expected to resolve conflicts in the working tree and register the
resolusion by doing "git add" before running "git merge --continue",
but I wonder if that is clear enough for new readers.
The same comment applies to the option description below. I suspect
that it is better to remove the last sentence above, leaving "4th
one can be run only with MERGE_HEAD" here, and enhance the
explanation in the option description (see below).
> OPTIONS
> -------
> @@ -99,6 +103,12 @@ commit or stash your changes before running 'git merge'.
> 'git merge --abort' is equivalent to 'git reset --merge' when
> `MERGE_HEAD` is present.
>
> +--continue::
> + Take the currently staged changes and complete the merge.
> ++
> +'git merge --continue' is equivalent to 'git commit' when
> +`MERGE_HEAD` is present.
> +
These two sentences are even more technical and unfriendly to new
readers, I am afraid. How about giving a hint by referring to an
existing section, like this?
--continue::
After a "git merge" stops due to conflicts you can conclude
the merge by running "git merge --continue" (see "How to
resolve conflicts" section below).
> @@ -277,7 +287,8 @@ After seeing a conflict, you can do two things:
>
> * Resolve the conflicts. Git will mark the conflicts in
> the working tree. Edit the files into shape and
> - 'git add' them to the index. Use 'git commit' to seal the deal.
> + 'git add' them to the index. Use 'git merge --continue' to seal the
> + deal.
Why do we want to make it harder to discover "git commit" here?
I would understand:
... Use 'git commit' to conclude (you can also say 'git
merge --continue').
though. After all, we are merely introducing a synonym for those
who want to type more. There is no plan to deprecate the use of
'git commit', which is a perfectly reasonable way to conclude an
interrupted merge, that has worked well for us for the past 10 years
and still works.
> @@ -65,6 +66,7 @@ static int option_renormalize;
> static int verbosity;
> static int allow_rerere_auto;
> static int abort_current_merge;
> +static int continue_current_merge;
> static int allow_unrelated_histories;
> static int show_progress = -1;
> static int default_to_upstream = 1;
> @@ -223,6 +225,8 @@ static struct option builtin_merge_options[] = {
> OPT__VERBOSITY(&verbosity),
> OPT_BOOL(0, "abort", &abort_current_merge,
> N_("abort the current in-progress merge")),
> + OPT_BOOL(0, "continue", &continue_current_merge,
> + N_("continue the current in-progress merge")),
> OPT_BOOL(0, "allow-unrelated-histories", &allow_unrelated_histories,
> N_("allow merging unrelated histories")),
> OPT_SET_INT(0, "progress", &show_progress, N_("force progress reporting"), 1),
> @@ -739,7 +743,7 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)
> if (err_msg)
> error("%s", err_msg);
> fprintf(stderr,
> - _("Not committing merge; use 'git commit' to complete the merge.\n"));
> + _("Not committing merge; use 'git merge --continue' to complete the merge.\n"));
Likewise. I do not see a need to change this one at all.
> @@ -1166,6 +1170,22 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> goto done;
> }
>
> + if (continue_current_merge) {
> + int nargc = 1;
> + const char *nargv[] = {"commit", NULL};
> +
> + if (argc)
> + usage_msg_opt("--continue expects no arguments",
> + builtin_merge_usage, builtin_merge_options);
Peff already commented on "what about other options?", and I think
his "check the number of args before parse-options ran to ensure
that the '--abort' or '--continue' was the only thing" is probably
a workable hack.
The "right" way to fix it would be way too involved to be worth for
just this single change (and also fixing "--abort"). Just thinking
aloud:
* Update parse-options API to:
- extend "struct option" with one field that holds what "command
modes" the option the "struct option" describes is incompatible
with.
- make parse_options() to keep track of the set of command modes
that are compatible with the options seen so far, and complain
if an option that is not compatible with the command mode is
given.
* Use the above facility to update "git merge" so that --abort and
--continue becomes OPT_CMDMODE.
Then, the updated parse_options() would:
- start by making the "incompatible command modes" an empty set.
- while it processes each option on the command line:
- if it is not an OPTION_CMDMODE, add the set of command modes
that are incompatible with the option to the "incompatible
command modes".
- if it is an OPTION_CMDMODE and we already saw another
OPTION_CMDMODE, error out (we already do this).
- after all options are read, check the final command mode and see
if that is in "incompatible command modes".
You can mark almost all options "git merge" takes except a selected
few like "--quiet" as incompatible with "--abort" and "--continue"
and let parse_options() catch incompatible options. Of course you
still need to check argc for non-option here.
^ permalink raw reply
* RE: [PATCH 6/6] rm: add absorb a submodules git dir before deletion
From: David Turner @ 2016-12-13 18:06 UTC (permalink / raw)
To: 'Stefan Beller', gitster@pobox.com
Cc: git@vger.kernel.org, bmwill@google.com
In-Reply-To: <20161213014055.14268-7-sbeller@google.com>
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Monday, December 12, 2016 8:41 PM
> To: gitster@pobox.com
> Cc: git@vger.kernel.org; David Turner; bmwill@google.com; Stefan Beller
> Subject: [PATCH 6/6] rm: add absorb a submodules git dir before deletion
>
> When deleting a submodule we need to keep the actual git directory around,
Nit: comma after submodule.
> - strbuf_reset(&buf);
> - strbuf_addstr(&buf, path);
> - if (!remove_dir_recursively(&buf, 0)) {
> - removed = 1;
> - if
> (!remove_path_from_gitmodules(path))
> - gitmodules_modified = 1;
> - strbuf_release(&buf);
> - continue;
> - } else if (!file_exists(path))
> - /* Submodule was removed by user */
> - if
> (!remove_path_from_gitmodules(path))
> - gitmodules_modified = 1;
> + if (file_exists(path))
> + depopulate_submodule(path);
> + removed = 1;
> + if (!remove_path_from_gitmodules(path))
> + gitmodules_modified = 1;
> + continue;
> /* Fallthrough and let remove_path() fail.
> */
It seems odd to have a continue right before a comment that says "Fallthrough".
^ permalink raw reply
* Re: [PATCH 1/2] alternates: accept double-quoted paths
From: Junio C Hamano @ 2016-12-13 18:08 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Jeff King, Johannes Sixt, Klaus Ethgen, Git Mailing List
In-Reply-To: <CACsJy8B52ZDRTUjGLqub_1wELtugv99xbDnBg1PX1LUTb6nVMQ@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> At least attr has the same problem and is going the same direction
> [1]. Cool. (I actually thought the patch was in and evidence that this
> kind of backward compatibility breaking was ok, turns out the patch
> has stayed around for years)
>
> [1] http://public-inbox.org/git/%3C20161110203428.30512-18-sbeller@google.com%3E/
Seeing that again, I see this in the proposed log message:
Full pattern must be quoted. So 'pat"t"ern attr' will give exactly
'pat"t"ern', not 'pattern'.
I cannot tell what it is trying to say. The code suggests something
quite different, i.e. a line like this
"pat\"t\"ern" attr
would tell us that a path that matches the pattern
pat"t"ern
is given 'attr'.
The log message may need to be cleaned up. The update by that
commit to the documentation looks alright, thoguh.
^ permalink raw reply
* Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates
From: Junio C Hamano @ 2016-12-13 18:10 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Klaus Ethgen, git
In-Reply-To: <20161213114414.masgfo7lf7e3utym@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The naive conversion is just:
> ...
> -# MINGW does not allow colons in pathnames in the first place
> -test_expect_success !MINGW 'push to repo path with colon' '
> +if test_have_prereq MINGW
> +then
> + path_sep=';'
> +else
> + path_sep=':'
> +fi
> ...
> - git clone --bare . xxx:yyy.git &&
> + git clone --bare . xxx${path_sep}yyy.git &&
Don't you want to dq the whole thing to prevent the shell from
splitting this into two commands at ';'? The other one below is OK.
>
> echo change >>file.bin &&
> git commit -am change &&
> # Note that we have to use the full path here, or it gets confused
> # with the ssh host:path syntax.
> - git push "$PWD/xxx:yyy.git" HEAD
> + git push "$PWD/xxx${path_sep}yyy.git" HEAD
> '
>
> test_done
>
> Does that work?
>
> -Peff
^ permalink raw reply
* Re: [PATCH 0/2] handling alternates paths with colons
From: Junio C Hamano @ 2016-12-13 18:10 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Klaus Ethgen, git
In-Reply-To: <20161213115018.quulwlheycjtlsub@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Dec 12, 2016 at 02:37:08PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > So here are patches that do that. It kicks in only when the first
>> > character of a path is a double-quote, and then expects the usual
>> > C-style quoting.
>>
>> The quote being per delimited component is what makes this fifth
>> approach a huge win.
>>
>> All sane components on a list-valued environment are still honored
>> and an insane component that has either a colon in the middle or
>> begins with a double-quote gets quoted. As long as nobody used a
>> path that begins with a double-quote as an element in such a
>> list-valued environment (and they cannot be, as using a non-absolute
>> path as an element does not make much sense), this will be safe, and
>> a path with a colon didn't work with Git unaware of the new quoting
>> rule anyway. Nice.
>
> We do support non-absolute paths, both in alternates files and
> environment variables. It's a nice feature if you want to have a
> relocatable family of shared repositories. I'd imagine that most cases
> start with "../", though.
Yes. I was only talking about the environment variable, as you can
tell from my mention of "colon" there.
^ permalink raw reply
* Re: [PATCH 1/4] doc: add articles (grammar)
From: Junio C Hamano @ 2016-12-13 18:11 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Philip Oakley, git
In-Reply-To: <8737hsj7wp.fsf@gmail.com>
Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com> writes:
> Thank you for reviewing this series, Philip.
>
> Philip Oakley writes:
>
>> This looks good to me.
>
> I'll add this header:
>
> Acked-by: Philip Oakley <philipoakley@iee.org>
>
> To the commit message of this patch in the next review round (version 2
> of the patch series).
>
> Let me know if I should add another header, or do something different
> than this.
I was planning to merge all four from you as-is to 'next' today,
though. Should I wait?
^ permalink raw reply
* Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates
From: Jeff King @ 2016-12-13 18:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Klaus Ethgen, git
In-Reply-To: <xmqqwpf34s5f.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 13, 2016 at 10:10:04AM -0800, Junio C Hamano wrote:
> > - git clone --bare . xxx:yyy.git &&
> > + git clone --bare . xxx${path_sep}yyy.git &&
>
> Don't you want to dq the whole thing to prevent the shell from
> splitting this into two commands at ';'? The other one below is OK.
After expansion, I don't think the shell will do any further processing
except for whitespace splitting. E.g.:
$ debug() { for i in "$@"; do echo "got $i"; done; }
$ sep=';'
$ space=' '
$ debug foo${sep}bar
got foo;bar
$ debug foo${space}bar
got foo
got bar
I don't mind quoting it to make it more obvious, though.
-Peff
^ permalink raw reply
* Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
From: Junio C Hamano @ 2016-12-13 18:15 UTC (permalink / raw)
To: Vasco Almeida
Cc: Johannes Schindelin, git, Jiang Xin,
Ævar Arnfjörð Bjarmason, Jean-Noël AVILA,
Jakub Narębski, David Aguilar
In-Reply-To: <1481627820.2041.21.camel@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
>> We only update comment_line_char from the default "#" when the
>> configured value is a single-byte character and we ignore incorrect
>> values in the configuration file. So I think the patch you sent is
>> correct after all.
>
> I am still not sure what version do we prefer.
>
> Check whether core.commentchar is a single character. If not, use '#'
> as the $comment_line_char.
This, plus special casing "auto".
Picking the first byte is inconsistent with the current practice
(the paragraph you quoted above), I think.
^ permalink raw reply
* Re: [PATCH 0/2] handling alternates paths with colons
From: Jeff King @ 2016-12-13 18:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Klaus Ethgen, git
In-Reply-To: <xmqqshpr4s41.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 13, 2016 at 10:10:54AM -0800, Junio C Hamano wrote:
> > We do support non-absolute paths, both in alternates files and
> > environment variables. It's a nice feature if you want to have a
> > relocatable family of shared repositories. I'd imagine that most cases
> > start with "../", though.
>
> Yes. I was only talking about the environment variable, as you can
> tell from my mention of "colon" there.
Right, but we also support relative paths via environment variables. I
don't think that changes the conclusion though. I'm not convinced
relative paths via the environment aren't slightly insane in the first
place, given the discussion in 37a95862c (alternates: re-allow relative
paths from environment, 2016-11-07). And they'd probably start with
"../" as well.
-Peff
^ permalink raw reply
* Re: [PATCH 0/2] handling alternates paths with colons
From: Junio C Hamano @ 2016-12-13 18:42 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Klaus Ethgen, git
In-Reply-To: <20161213181755.wrgu6drm45v7xhnl@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Right, but we also support relative paths via environment variables. I
> don't think that changes the conclusion though. I'm not convinced
> relative paths via the environment aren't slightly insane in the first
> place,
Sorry, a triple negation is above my head. "relative paths in env
aren't insane" == "using relative paths is OK" and you are not
convinced that it is the case, so you are saying that you think
relative paths in env is not something that is sensible?
> given the discussion in 37a95862c (alternates: re-allow relative
> paths from environment, 2016-11-07). And they'd probably start with
> "../" as well.
Yeah. In any case, there is a potential regression but the chance
is miniscule---the user has to have been using a relative path that
begins with a double-quote in the environment or in alternates file.
^ permalink raw reply
* Re: [PATCH 0/2] handling alternates paths with colons
From: Jeff King @ 2016-12-13 18:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Klaus Ethgen, git
In-Reply-To: <xmqqbmwf4qn4.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 13, 2016 at 10:42:39AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Right, but we also support relative paths via environment variables. I
> > don't think that changes the conclusion though. I'm not convinced
> > relative paths via the environment aren't slightly insane in the first
> > place,
>
> Sorry, a triple negation is above my head. "relative paths in env
> aren't insane" == "using relative paths is OK" and you are not
> convinced that it is the case, so you are saying that you think
> relative paths in env is not something that is sensible?
I think relative paths in env have very flaky semantics which makes them
inadvisable to use in general. That being said, when we broke even those
flaky semantics somebody complained. So I consider a feature we _do_
support, but not one I would recommend to people.
-Peff
^ permalink raw reply
* Re: git add -p with new file
From: Junio C Hamano @ 2016-12-13 18:48 UTC (permalink / raw)
To: Jeff King; +Cc: Stephan Beyer, Ariel, git
In-Reply-To: <20161213173341.wemlunlixdp6277h@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> I am also not really sure what problem this feature is trying to solve.
>> If the "problem"(?) is that it should act more like "git add" instead of
>> "git add -u", for whatever reason, this may be fine (but the
>> configuration option is a must-have then).
>
> I think the problem is just that "add -p" does not give the whole story
> of what you might want to do before making a commit.
The same is shared by "git diff [HEAD]", by the way. It is beyond
me why people use "add -p", not "git diff [HEAD]", for the final
sanity check before committing.
Perhaps the latter is not advertised well enough? "add -p" does not
even page so it is not very useful way to check what is being added
if you are adding a new file (unless you are doing a toy example to
add a 7-line file).
>> > I'd also probably add interactive.showUntracked to make the whole thing
>> > optional (but I think it would be OK to default it to on).
>> Hm, "interactive.showUntracked" is a confusing name because "git add -i"
>> (interactive) already handles untracked files.
>
> Sure, that was just meant for illustration. I agree there's probably a
> better name.
"interactive.*" is not a sensible hierarchy to use, because things
other than "add" can go interactive.
addPatch.showUntracked?
^ 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