* Re: [PATCH v4 4/4] parse: separate out parsing functions from config.h
From: Phillip Wood @ 2023-10-10 17:58 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Calvin Wan, Junio C Hamano
In-Reply-To: <20231010174348.2150150-1-jonathantanmy@google.com>
On 10/10/2023 18:43, Jonathan Tan wrote:
> phillip.wood123@gmail.com writes:
>> Hi Jonathan
>>
>> On 29/09/2023 22:20, Jonathan Tan wrote:
>>> diff --git a/parse.h b/parse.h
>>> new file mode 100644
>>> index 0000000000..07d2193d69
>>> --- /dev/null
>>> +++ b/parse.h
>>> @@ -0,0 +1,20 @@
>>> +#ifndef PARSE_H
>>> +#define PARSE_H
>>> +
>>> +int git_parse_signed(const char *value, intmax_t *ret, intmax_t max);
>>
>> Previously this function was private to config.c, now it needs to be
>> public because it is still called by
>> git_config_get_expiry_date_in_days(). As this is essentially an internal
>> helper for git_parse_int() and friends it is a bit unfortunate that it
>> is now public. Perhaps we should change
>> git_config_get_expiry_date_in_days() to call git_parse_int() instead.
>> Then we can keep git_parse_signed() and git_parse_unsigned() private to
>> parse.c.
>
> It could be argued also that it fits in with the rest of
> the parsing functions - this one parses intmax, and we have
> others of various signedness and size.
This one differs from the others because it expects the caller to pass a
maximum value, the intmax_t equivalent to git_parse_int() would be
int git_parse_intmax(const char*, intmax_t*);
We now expose git_parse_int64() which covers a similar case.
> I'm open to changing
> git_config_get_expiry_date_in_days() too, though...we probably don't
> need so many days.
Indeed, the existing code passes maximum_signed_value_of_type(int) as
the third argument to limit it to INT_MAX already.
>>> +/**
>>> + * Same as `git_config_bool`, except that it returns -1 on error rather
>>> + * than dying.
>>> + */
>>> +int git_parse_maybe_bool(const char *);
>>> +int git_parse_maybe_bool_text(const char *value);
>>
>> This used to be private to config.c and now has callers in parse.c and
>> config.c. We should make it clear that non-config code is likely to want
>> git_parse_maybe_bool() rather than this function.
>>
>> Best Wishes
>>
>> Phillip
>
> The difference between these 2 functions here is that bool_text supports
> only the textual forms (used, for example, in git_config_bool_or_int()
> which accepts both boolean strings and integers), which might be useful
> elsewhere too. But it could be better documented, yes.
>
> Looking at "What's Cooking", this series is about to be merged to
> master. We could hold off merging that, but I think we don't need to
> - it could be argued that git_parse_maybe_bool_text() could be better
> documented, but even if we wrote it from scratch, I would probably put
> the extra documentation in its own patch anyway (so one patch for moving
> the code, and another for adding documentation).
I agree it's not worth re-rolling just to add some documentation here.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH] doc/git-worktree: mention "refs/rewritten" as per-worktree refs
From: Eric Sunshine @ 2023-10-10 17:49 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Johannes Schindelin
In-Reply-To: <985ac850eb6e60ae76601acc8bfbcd56f99348b4.1696935657.git.ps@pks.im>
On Tue, Oct 10, 2023 at 7:01 AM Patrick Steinhardt <ps@pks.im> wrote:
> Some references are special in the context of worktrees as they are
> considered to be per-worktree instead of shared across all of the
> worktrees. Most importantly, this includes "refs/worktree/" that have
> explicitly been designed such that users can create per-woorktree refs.
s/woorktree/worktree/
> But there are also special references that have an associated meaning
> like "refs/bisect/", which is used to track state of git-bisect(1).
>
> These special per-worktree references are documented in git-worktree(1),
> but one instance is missing. In a9be29c9817 (sequencer: make refs
> generated by the `label` command worktree-local, 2018-04-25), we have
> converted "refs/rewritten/" to be a per-worktree reference as well.
> These references are used by our sequencer infrastructure to generate
> labels for rebased commits. So in order to allow for multiple concurrent
> rebases to happen in different worktrees, these references need to be
> tracked per worktree.
>
> We forgot to update our documentation to mention these new per-worktree
> references, which is fixed by this patch.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -286,7 +286,8 @@ rules and how to access refs of one worktree from another.
> In general, all pseudo refs are per-worktree and all refs starting with
> `refs/` are shared. Pseudo refs are ones like `HEAD` which are directly
> under `$GIT_DIR` instead of inside `$GIT_DIR/refs`. There are exceptions,
> -however: refs inside `refs/bisect` and `refs/worktree` are not shared.
> +however: refs inside `refs/bisect`, `refs/worktree` and `refs/rewritten` are
> +not shared.
To simplify future maintenance, eventually we might want to turn this
into a bulleted list, but that doesn't necessarily have to be done by
this patch, which is fine as-is.
> @@ -363,8 +364,8 @@ linked worktree `git rev-parse --git-path HEAD` returns
> `$GIT_COMMON_DIR` and returns `/path/main/.git/refs/heads/master`,
> -since refs are shared across all worktrees, except `refs/bisect` and
> -`refs/worktree`.
> +since refs are shared across all worktrees, except `refs/bisect`,
> +`refs/worktree` and `refs/rewritten`.
Ditto.
^ permalink raw reply
* Re: [PATCH v4 4/4] parse: separate out parsing functions from config.h
From: Jonathan Tan @ 2023-10-10 17:43 UTC (permalink / raw)
To: phillip.wood123; +Cc: Jonathan Tan, git, Calvin Wan, Junio C Hamano
In-Reply-To: <1de0a6f3-e223-4e84-a6d2-51d9b51a02f6@gmail.com>
phillip.wood123@gmail.com writes:
> Hi Jonathan
>
> On 29/09/2023 22:20, Jonathan Tan wrote:
> > diff --git a/parse.h b/parse.h
> > new file mode 100644
> > index 0000000000..07d2193d69
> > --- /dev/null
> > +++ b/parse.h
> > @@ -0,0 +1,20 @@
> > +#ifndef PARSE_H
> > +#define PARSE_H
> > +
> > +int git_parse_signed(const char *value, intmax_t *ret, intmax_t max);
>
> Previously this function was private to config.c, now it needs to be
> public because it is still called by
> git_config_get_expiry_date_in_days(). As this is essentially an internal
> helper for git_parse_int() and friends it is a bit unfortunate that it
> is now public. Perhaps we should change
> git_config_get_expiry_date_in_days() to call git_parse_int() instead.
> Then we can keep git_parse_signed() and git_parse_unsigned() private to
> parse.c.
It could be argued also that it fits in with the rest of
the parsing functions - this one parses intmax, and we have
others of various signedness and size. I'm open to changing
git_config_get_expiry_date_in_days() too, though...we probably don't
need so many days.
> > +/**
> > + * Same as `git_config_bool`, except that it returns -1 on error rather
> > + * than dying.
> > + */
> > +int git_parse_maybe_bool(const char *);
> > +int git_parse_maybe_bool_text(const char *value);
>
> This used to be private to config.c and now has callers in parse.c and
> config.c. We should make it clear that non-config code is likely to want
> git_parse_maybe_bool() rather than this function.
>
> Best Wishes
>
> Phillip
The difference between these 2 functions here is that bool_text supports
only the textual forms (used, for example, in git_config_bool_or_int()
which accepts both boolean strings and integers), which might be useful
elsewhere too. But it could be better documented, yes.
Looking at "What's Cooking", this series is about to be merged to
master. We could hold off merging that, but I think we don't need to
- it could be argued that git_parse_maybe_bool_text() could be better
documented, but even if we wrote it from scratch, I would probably put
the extra documentation in its own patch anyway (so one patch for moving
the code, and another for adding documentation).
^ permalink raw reply
* Re: [PATCH v4 2/4] wrapper: reduce scope of remove_or_warn()
From: Jonathan Tan @ 2023-10-10 17:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, phillip.wood123, git, Calvin Wan
In-Reply-To: <xmqqjzruv4k1.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> phillip.wood123@gmail.com writes:
>
> > Hi Jonathan
> >
> > On 29/09/2023 22:20, Jonathan Tan wrote:
> >> From: Calvin Wan <calvinwan@google.com>
> >> remove_or_warn() is only used by entry.c and apply.c, but it is
> >> currently declared and defined in wrapper.{h,c}, so it has a scope much
> >> greater than it needs. This needlessly large scope also causes wrapper.c
> >> to need to include object.h, when this file is largely unconcerned with
> >> Git objects.
> >> Move remove_or_warn() to entry.{h,c}. The file apply.c still has
> >> access
> >> to it, since it already includes entry.h for another reason.
> >
> > This looks good. On a related note wrapper.c includes repository.h but
> > does use anything declared in that header.
> >
> > Best Wishes
> >
> > Phillip
>
> Thanks for a review. I just checked 'master', 'next', and 'seen'
> and in all '#include <repository.h>' can safely be dropped from
> there, it seems. It may be too trivial even for a microproject,
> but nevertheless a nice clean-up.
Ah, Calvin fixed this in one of the subsequent patches, but I'll put it
into its own patch in my updated version.
^ permalink raw reply
* Re: [PATCH 0/3] rev-list: add support for commits in `--missing`
From: Junio C Hamano @ 2023-10-10 17:09 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git
In-Reply-To: <ZSTs3BUVtaI9QIoA@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> I had already reviewed the patches internally at GitLab, so for what
> it's worth please feel free to add my Reviewed-by.
Great. It seems that 'seen' with this series fails to pass the
tests, though.
https://github.com/git/git/actions/runs/6462854176/job/17545104753
Thanks.
>
> Patrick
>
>> Karthik Nayak (3):
>> revision: rename bit to `do_not_die_on_missing_objects`
>> rev-list: move `show_commit()` to the bottom
>> rev-list: add commit object support in `--missing` option
>>
>> builtin/reflog.c | 2 +-
>> builtin/rev-list.c | 93 +++++++++++++++++++------------------
>> list-objects.c | 2 +-
>> object.h | 2 +-
>> revision.c | 9 +++-
>> revision.h | 20 ++++----
>> t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++
>> 7 files changed, 145 insertions(+), 57 deletions(-)
>> create mode 100755 t/t6022-rev-list-missing.sh
>>
>> --
>> 2.41.0
>>
^ permalink raw reply
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Junio C Hamano @ 2023-10-10 17:07 UTC (permalink / raw)
To: Josh Triplett; +Cc: Kristoffer Haugsbakk, Sebastian Thiel, git
In-Reply-To: <ZSVbUSRUQlNy0bj-@localhost>
Josh Triplett <josh@joshtriplett.org> writes:
> While I'd love for it to default to that and require an extra option to
> clean away precious files, I'd expect that that would break people's
> workflows and finger memory. If someone expects `git clean -x -d -f` to
> clean away everything, including `.config`, and then it leaves some
> files in place, that seems likely to cause problems. (Leaving aside that
> it might break scripted workflows.)
I thought the point of introducing the new "precious" class of
paths, in addition to the current "tracked", "ignored, untracked,
and expendable", "not ignored and untracked", is so that people can
do "git clean -x -d -f" and expect the ".config" that is marked as
"precious" to stay. Before their Git learned the precious class, if
they marked ".config" as "ignored, untracked, and expendable", then
such an invocation of "clean" would have removed it, but if they add
it to the new "precious" class, their expectation ought to be that
precious ones are not removed, no? Otherwise I am not quite sure
what the point of adding such a new protection is.
^ permalink raw reply
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Junio C Hamano @ 2023-10-10 17:02 UTC (permalink / raw)
To: Sebastian Thiel; +Cc: git, Josh Triplett
In-Reply-To: <79901E6C-9839-4AB2-9360-9EBCA1AAE549@icloud.com>
Sebastian Thiel <sebastian.thiel@icloud.com> writes:
> I'd like to propose adding a new standard gitattribute "precious".
;-).
Over the years, I've seen many times scenarios that would have been
helped if we had not just "tracked? ignored? unignored?" but also
the fourth kind [*]. The word "ignored" (or "excluded") has always
meant "not tracked, not to be tracked, and expendable" to Git, and
"ignored but unexpendable" class was missing. I even used the term
"precious" myself in those discussions. At the concept level, I
support the effort 100%, but as always, the devil will be in the
details.
Scenarios that people wished for "precious" traditionally have been
* You are working on 'master'. You have in your .gitignore or
.git/info/exclude a line to ignore path A, and have random
scribbles in a throw-away file there. There is another branch
'seen', where they added some tracked contents at path A/B. You
do "git checkout seen" and your file A that is an expendable file,
because it is listed as ignored in .git/info/exclude, is removed
to make room for creating A/B.
* Similar situation, but this time, 'seen' branch added a tracked
contents at path A. Again, "git checkout seen" will discard the
expendable file A and replace it with tracked contents.
* Instead of "git checkout", you decide to merge the branch 'seen'
to the checkout of 'master', where you have an ignored path A.
Because merging 'seen' would need to bring the tracked contents
of either A/B (in the first scenario above) or A (in the second
scenario), your "expendable" A will be removed to make room.
In previous discussions, nobody was disturbed that "git clean" was
unaware of the "precious" class, but if we were to have the
"precious" class in addition to "ignored" aka "expendable", I would
not oppose to teach "git clean" about it, too.
There was an early and rough design draft there in
https://lore.kernel.org/git/7vipsnar23.fsf@alter.siamese.dyndns.org/
which probably is worth a read, too.
Even though I referred to the precious _attribute_ in some of these
discussions, between the attribute mechanism and the ignore
mechanism, I am actually leaning toward suggesting to extend the
exclude/ignore mechanism to introduce the "precious" class. That
way, we can avoid possible snafu arising from marking a path in
.gitignore as ignored, and in .gitattrbutes as precious, and have to
figure out how these two settings are to work together.
In any case, the "precious" paths are expected to be small minority
of what people never want to "git add" or "git commit", so coming up
with a special syntax to be used in .gitignore, even if that special
syntax is ugly and cumbersome to type, would be perfectly OK.
[Reference]
* https://lore.kernel.org/git/7viptp9jos.fsf@alter.siamese.dyndns.org/
* https://lore.kernel.org/git/xmqqva534vnb.fsf@gitster-ct.c.googlers.com/
^ permalink raw reply
* Re: [PATCH v4 0/4] Preliminary patches before git-std-lib
From: Jonathan Tan @ 2023-10-10 16:21 UTC (permalink / raw)
To: phillip.wood123; +Cc: Jonathan Tan, git, Calvin Wan, Junio C Hamano
In-Reply-To: <4670774d-a899-492c-9b36-98ee243c8d4d@gmail.com>
phillip.wood123@gmail.com writes:
> Hi Jonathan
>
> On 29/09/2023 22:20, Jonathan Tan wrote:
> > Calvin will be away for a few weeks and I'll be handling the git-std-lib
> > effort in the meantime. My goals will be:
> >
> > - Get the preliminary patches in Calvin's patch set (patches 1-4) merged
> > first.
> >
> > - Updating patches 5-6 based on reviewer feedback (including my
> > feedback). I have several aims including reducing or eliminating the
> > need for the GIT_STD_LIB preprocessor variable, and making stubs a test-
> > only concern (I think Phillip has some similar ideas [1] but I haven't
> > looked at their repo on GitHub yet).
>
> It sounds like we're thinking along similar lines, do feel free get in
> touch on or off the list if you want to ask anything about those patches
> I pushed to github.
Thanks. I'm updating patches 5-6 now and basing on your work, in fact.
> > [1] https://lore.kernel.org/git/98f3edcf-7f37-45ff-abd2-c0038d4e0589@gmail.com/
> >
> > This patch set is in service of the first goal. Because the libification
> > patches are no longer included in this patch set, I have rewritten the
> > commit messages to justify the patches in terms of code organization.
> > There are no changes in the code itself. Also, I have retained Calvin's
> > name as the author.
>
> I agree it makes sense to get the preliminary patches merged on their
> own. I think the argument that they reduce the scope of includes is a
> reasonable justification on its own. I've left a couple of comments but
> they're looking pretty good.
>
> Best Wishes
>
> Phillip
Thanks.
^ permalink raw reply
* Re: [PATCH v4 2/4] wrapper: reduce scope of remove_or_warn()
From: Junio C Hamano @ 2023-10-10 16:13 UTC (permalink / raw)
To: phillip.wood123; +Cc: Jonathan Tan, git, Calvin Wan
In-Reply-To: <066b3162-6a81-45d7-b164-17b74e6c92dc@gmail.com>
phillip.wood123@gmail.com writes:
> Hi Jonathan
>
> On 29/09/2023 22:20, Jonathan Tan wrote:
>> From: Calvin Wan <calvinwan@google.com>
>> remove_or_warn() is only used by entry.c and apply.c, but it is
>> currently declared and defined in wrapper.{h,c}, so it has a scope much
>> greater than it needs. This needlessly large scope also causes wrapper.c
>> to need to include object.h, when this file is largely unconcerned with
>> Git objects.
>> Move remove_or_warn() to entry.{h,c}. The file apply.c still has
>> access
>> to it, since it already includes entry.h for another reason.
>
> This looks good. On a related note wrapper.c includes repository.h but
> does use anything declared in that header.
>
> Best Wishes
>
> Phillip
Thanks for a review. I just checked 'master', 'next', and 'seen'
and in all '#include <repository.h>' can safely be dropped from
there, it seems. It may be too trivial even for a microproject,
but nevertheless a nice clean-up.
^ permalink raw reply
* Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635
From: Naomi Ibe @ 2023-10-10 15:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqlecbzl5e.fsf@gitster.g>
Thank you very much! I'd definitely make those changes on my next patch.
Should I begin work on version 2 or should I still wait for additional
input on the version 1?
On Mon, Oct 9, 2023 at 7:49 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Naomi Ibe <naomi.ibeh69@gmail.com> writes:
>
> > Subject: Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using d
>
> Subject: [OUTREACHY][PATCH 1/1] builtin/add.c: clean up die() messages
>
> > From: Naomi <naomi.ibeh69@gmail.com>
>
> The name and address on this line come from your commit object,
> which in turn would have came from your configuration. You have
>
> [user]
> name = Naomi
> email = naomi.ibeh69@gmail.com
>
> somewhere in your configuration file, perhaps in $HOME/.gitconfig or
> somewhere. When contributiong to this project, you want that "name"
> line to also include your family name, as it should match what you
> write on your Signed-off-by: line. A focused way to do so without
> affecting your author identity for other projects is to add
>
> [user]
> name = Naomi Ibe
>
> in .git/config of the repository that you use to contribute to this
> project, e.g.,
>
> $ cd ... to the working tree of your clone of git you work in ...
> $ git config user.name "Naomi Ibe"
>
> The space above your Sign off is to fill the details you omitted on
> the title of the message (which would say something about "conform
> to guidelines" or "clean up" without mentioning what rule the
> original violates), making the whole thing something like:
>
> builtin/add.c: clean up die() messages
>
> As described in the CodingGuidelines document, a single line
> message given to die() and its friends should not capitalize its
> first word, and should not add full-stop at the end.
>
> Signed-off-by: ...
>
> > Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
> > ---
> > builtin/add.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
>
> Thanks. Otherwise the patch looks good.
>
> >
> > diff --git a/builtin/add.c b/builtin/add.c
> > index c27254a5cd..5126d2ede3 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -182,7 +182,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
> > git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
> >
> > if (repo_read_index(the_repository) < 0)
> > - die(_("Could not read the index"));
> > + die(_("could not read the index"));
> >
> > repo_init_revisions(the_repository, &rev, prefix);
> > rev.diffopt.context = 7;
> > @@ -200,15 +200,15 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
> > die(_("editing patch failed"));
> >
> > if (stat(file, &st))
> > - die_errno(_("Could not stat '%s'"), file);
> > + die_errno(_("could not stat '%s'"), file);
> > if (!st.st_size)
> > - die(_("Empty patch. Aborted."));
> > + die(_("empty patch. aborted"));
> >
> > child.git_cmd = 1;
> > strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
> > NULL);
> > if (run_command(&child))
> > - die(_("Could not apply '%s'"), file);
> > + die(_("could not apply '%s'"), file);
> >
> > unlink(file);
> > free(file);
> > @@ -568,7 +568,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> > finish:
> > if (write_locked_index(&the_index, &lock_file,
> > COMMIT_LOCK | SKIP_IF_UNCHANGED))
> > - die(_("Unable to write new index file"));
> > + die(_("unable to write new index file"));
> >
> > dir_clear(&dir);
> > clear_pathspec(&pathspec);
^ permalink raw reply
* Re: [PATCH v3] merge-ort: initialize repo in index state
From: John Cai @ 2023-10-10 15:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Cai via GitGitGadget, git, Elijah Newren
In-Reply-To: <xmqq4jizxyla.fsf@gitster.g>
Hi Junio
On 9 Oct 2023, at 17:41, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> Fix this by initializing the repository in the index state.
>>
>> Changes since V2:
>>
>> * fixed test by using printf instead of echo
>
> Much better than using unportable \n with echo.
>
>> -+ echo "foo\nbar\nbaz" >expect &&
>> ++ printf "foo\nbar\nbaz\n" >expect &&
>
> But if we are using printf, it would be easier to read lines
> separately, which would look more like
>
> printf "%s\n" foo bar baz >expect
>
> And we have
>
> test_write_lines foo bar baz >expect
>
> to make it even more discoverable.
wasn't aware of test_write_lines, thanks.
>
>> + git cat-file -p "$tree:file1" >actual &&
>> + test_cmp expect actual
>> + )
>>
>>
>> merge-ort.c | 1 +
>> t/t4300-merge-tree.sh | 27 +++++++++++++++++++++++++++
>> 2 files changed, 28 insertions(+)
>>
>> diff --git a/merge-ort.c b/merge-ort.c
>> index 7857ce9fbd1..36537256613 100644
>> --- a/merge-ort.c
>> +++ b/merge-ort.c
>> @@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)
>> struct index_state *attr_index = &opt->priv->attr_index;
>> struct cache_entry *ce;
>>
>> + attr_index->repo = opt->repo;
>> attr_index->initialized = 1;
>>
>> if (!opt->renormalize)
>> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
>> index 57c4f26e461..c3a03e54187 100755
>> --- a/t/t4300-merge-tree.sh
>> +++ b/t/t4300-merge-tree.sh
>> @@ -86,6 +86,33 @@ EXPECTED
>> test_cmp expected actual
>> '
>>
>> +test_expect_success '3-way merge with --attr-source' '
>> + test_when_finished rm -rf 3-way &&
>> + git init 3-way &&
>> + (
>> + cd 3-way &&
>> + test_commit initial file1 foo &&
>> + base=$(git rev-parse HEAD) &&
>> + git checkout -b brancha &&
>> + echo bar >>file1 &&
>> + git commit -am "adding bar" &&
>> + source=$(git rev-parse HEAD) &&
>> + git checkout @{-1} &&
>> + git checkout -b branchb &&
>> + echo baz >>file1 &&
>> + git commit -am "adding baz" &&
>> + merge=$(git rev-parse HEAD) &&
>> + git checkout -b gitattributes &&
>> + test_commit "gitattributes" .gitattributes "file1 merge=union" &&
>
> OK, the branch "gitattributes" will be used to drive merge of file1
> using the union merge to avoid conflicting.
>
>> + git checkout @{-1} &&
>
> But such attribute will only be available in that branch, not in the
> checked out working tree. And then
>
>> + tree=$(git --attr-source=gitattributes merge-tree --write-tree \
>> + --merge-base "$base" --end-of-options "$source" "$merge") &&
>
> we use the gitattributes branch as the tree-ish to take the
> attribute information from. Makes sense.
>
>> + printf "foo\nbar\nbaz\n" >expect &&
>
> I'll squash in the "test_write_lines" change while queuing.
thank you!
John
>
>> + git cat-file -p "$tree:file1" >actual &&
>> + test_cmp expect actual
>> + )
>> +'
>> +
>> test_expect_success 'file change A, B (same)' '
>> git reset --hard initial &&
>> test_commit "change-a-b-same-A" "initial-file" "AAA" &&
>>
>> base-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09
>
> Thanks. Looking good.
^ permalink raw reply
* Re: [silly] worldview documents?
From: Emily Shaffer @ 2023-10-10 14:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren, git
In-Reply-To: <xmqqo7h7urgd.fsf@gitster.g>
On Mon, Oct 9, 2023 at 7:44 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> >> [Footnote]
> >> ...
> >
> > Thanks for writing this up. In the past, I didn't know how to put
> > into words why I didn't particularly care for this mode. You explain
> > it rather well.
>
> I am glad it helped non-zero number of people.
>
> It probably owes big to my failing, but because we strongly view it
> a virtue not to be opinionated, we have many discrete tools and
> features that can be used in combination to support a workflow well,
> even when some of these tools and features are not useful for a
> different workflow. It indeed is a good thing to be flexible and to
> support different workflows well, and we tend not to single out a
> workflow among many and advocate it, but because our documentation
> lacks description of major possible workflows, what their underlying
> philosophies and their strengths are, how some of our tools and
> features support them, and why some others are not good fit. Being
> given a toolbox with too many tools without being taught how they
> are to be used together and for what purpose may be a fun puzzle to
> figure out for tinkerers, but when you have a problem to solve and
> tinkering is not your main focus, which is true for most people, it
> is not fun.
>
> In short, in pursuit of not to be opinionated, we fail to give the
> readers best current practices. The first place to start rectifying
> it might be to have some write-ups for various major workflows and
> the worldview behind them.
I completely agree with you. The #1 conversation I have with friends
who are new to Git is figuring out which of the major workflows they
should use, and what are the drawbacks and benefits. And how to
diagnose based on the existing commit history, review tool in use, etc
which one is used by their peers. Having this documented somewhere -
maybe in Pro Git book, maybe in manpage - would be hugely useful. I
could envision a diagram of a sample commit history, and "if it
already looks like this or you want it to look like this, your
workflow should be blah" and finally a list of some drawbacks,
benefits, and warnings about what to avoid in that workflow. Plus,
perhaps, many shout-outs to `git reflog` for fixing when you
misunderstood and made a mess. (that's the #2 conversation I have :) )
> The importance given to first-parenthood
> offers two quite different worldviews that affects the choice of
> tools (e.g. "merge --no-ff", "checkout origin/master && merge mine
> && branch -f mine" aka "reverse merge").
>
> I suspect that this also relates to your "would --cc be totally
> unnecessary now we have --remerge-diff?" as well. What kind of
> conflicts are interesting highly depends on what you are looking
> for, which in turn is influenced by the workflow employed by the
> project and what role you are playing in it.
^ permalink raw reply
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Josh Triplett @ 2023-10-10 14:10 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Sebastian Thiel, git
In-Reply-To: <98387b86-1732-42bc-9ac5-d64a6617b2db@app.fastmail.com>
On Tue, Oct 10, 2023 at 03:38:51PM +0200, Kristoffer Haugsbakk wrote:
> Hi Sebastian
>
> On Tue, Oct 10, 2023, at 14:37, Sebastian Thiel wrote:
> > This highlights precious files by calling them out, but doesn't change the
> > behaviour of existing flags. Instead, the new flag `-p` is added which lets
> > `git clean` spare precious files.
>
> Why can't `clean` preserve precious files by default? And then delete them
> as well with something like `--no-keep-precious`? Is there some backwards
> compatibility concern?
While I'd love for it to default to that and require an extra option to
clean away precious files, I'd expect that that would break people's
workflows and finger memory. If someone expects `git clean -x -d -f` to
clean away everything, including `.config`, and then it leaves some
files in place, that seems likely to cause problems. (Leaving aside that
it might break scripted workflows.)
It seems safer to keep the existing behavior for existing options, and
add a new option for "remove everything except precious files".
^ permalink raw reply
* Re: [PATCH v3 11/15] replay: use standard revision ranges
From: Dragan Simic @ 2023-10-10 14:02 UTC (permalink / raw)
To: Christian Couder
Cc: Toon Claes, git, Junio C Hamano, Patrick Steinhardt,
Johannes Schindelin, Elijah Newren, John Cai, Derrick Stolee,
Phillip Wood, Felipe Contreras, Calvin Wan, Christian Couder
In-Reply-To: <CAP8UFD213MwZmBuK0At5r0O8tHYDnxRTkw9AHak9RwDBdVVq_A@mail.gmail.com>
On 2023-10-10 14:44, Christian Couder wrote:
> On Thu, Sep 7, 2023 at 11:02 PM Dragan Simic <dsimic@manjaro.org>
> wrote:
>>
>> On 2023-09-07 10:32, Christian Couder wrote:
>> > On Thu, Jun 22, 2023 at 12:03 PM Toon Claes <toon@iotcl.com> wrote:
>> >>
>> >> Christian Couder <christian.couder@gmail.com> writes:
>> >>
>> >> > +DESCRIPTION
>> >> > +-----------
>> >> > +
>> >> > +Takes a range of commits, and replays them onto a new location. Does
>> >> > +not touch the working tree or index, and does not update any
>> >> > +references. However, the output of this command is meant to be used
>> >>
>> >> Small suggestion here:
>> >>
>> >> Takes a range of commits, and replays them onto a new location. Does
>> >> neither touch the working tree nor index, and does not update any
>> >> references.
>> >
>> > I am not a native speaker, so I am not sure what's best here. I find
>> > your suggestion a bit less clear though, so until a native speaker
>> > agrees with it or maybe finds something even better, I prefer to leave
>> > it as-is.
>>
>> I'm also not a native English speaker, but I spent about 2.5 years
>> contributing a whole lot to English Wikipedia, so I'd dare to say I've
>> honed my English skills rather well. Thus, here's my take on this:
>>
>> Takes a range of commits and replays them onto a new location.
>> Leaves the working tree and the index untouched, and updates no
>> references. The output of this command is to be used...
>>
>> This is written in a concise yet slightly imperative way, which should
>> be suitable for the purpose. I hope you agree.
>
> I agree and I like it, so I have changed it to the above in version 5
> I just sent.
Great, thanks.
^ permalink raw reply
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Kristoffer Haugsbakk @ 2023-10-10 13:38 UTC (permalink / raw)
To: Sebastian Thiel; +Cc: Josh Triplett, git
In-Reply-To: <79901E6C-9839-4AB2-9360-9EBCA1AAE549@icloud.com>
Hi Sebastian
On Tue, Oct 10, 2023, at 14:37, Sebastian Thiel wrote:
> This highlights precious files by calling them out, but doesn't change the
> behaviour of existing flags. Instead, the new flag `-p` is added which lets
> `git clean` spare precious files.
Why can't `clean` preserve precious files by default? And then delete them
as well with something like `--no-keep-precious`? Is there some backwards
compatibility concern?
^ permalink raw reply
* Re: [PATCH v4 00/15] Introduce new `git replay` command
From: Christian Couder @ 2023-10-10 12:50 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes
In-Reply-To: <d07487c2-4ee6-3ffa-014d-418793a5a584@gmx.de>
Hi Dscho,
On Thu, Sep 7, 2023 at 1:04 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Christian,
>
> hope you had a restful vacation!
Yes, thanks! I hope you had a good summer too.
> I left a bit of feedback and think that once my concerns are addressed, a
> v5 will be ready for `next`.
Thanks for your review!
I think I have addressed most (if not all) of your concerns in version 5.
^ permalink raw reply
* Re: [PATCH v4 12/15] replay: disallow revision specific options and pathspecs
From: Christian Couder @ 2023-10-10 12:49 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Christian Couder
In-Reply-To: <f0e75d47-c277-9fbb-7bcd-53e4e5686f3c@gmx.de>
Hi Dscho,
On Thu, Sep 7, 2023 at 1:03 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> As pointed out elsewhere in this mail thread, I consider this patch to do
> more harm than good. After switching the command to plumbingmanipulators
> it should be possible to just forego all command-line validation and leave
> that job to the caller.
>
> Therefore I would love to see this patch dropped.
Ok, I have dropped this patch in version 5.
^ permalink raw reply
* Re: [PATCH v4 11/15] replay: use standard revision ranges
From: Christian Couder @ 2023-10-10 12:49 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Christian Couder
In-Reply-To: <03460733-0219-c648-5757-db1958f8042e@gmx.de>
On Thu, Sep 7, 2023 at 1:02 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Christian,
>
> It is a bit surprising to see the manual page added in _this_ patch, in
> the middle of the series... I can live with it, though.
It's explained in the cover letter and a bit in some commit messages.
For example in the "Possibly controversial issues" section of the
cover letter, there is:
"
* when and where to add tests and docs: although t6429 has tests that
are changed to use the new command instead of the fast-rebase
test-tool command as soon as the former is introduced, there is no
specific test script and no doc for the new command until commit
11/15 when standard revision ranges are used. This is done to avoid
churn in tests and docs while the final form of the command hasn't
crystalized enough. Adding tests and doc at this point makes this
commit quite big and possibly more difficult to review than if they
were in separate commits though. On the other hand when tests and
docs are added in specific commits, some reviewers say it would be
better to introduce them when the related changes are made.
"
> On Thu, 7 Sep 2023, Christian Couder wrote:
> > +SYNOPSIS
> > +--------
> > +[verse]
> > +'git replay' --onto <newbase> <revision-range>...
>
> We need to make it clear here, already in the SYNOPSIS, that this is
> experimental. Let's add an `(EXPERIMENTAL!)` prefix here.
I haven't done that as other commands marked as EXPERIMENTAL don't
have that in their SYNOPSIS.
> > [...]
> > diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
> > new file mode 100755
>
> Just like the manual page, I would have expected this test to be
> introduced earlier, and not piggy-backed onto one of the handful "let's
> turn fast-rebase into replay" patches.
This is discussed above.
^ permalink raw reply
* Re: [PATCH v4 11/15] replay: use standard revision ranges
From: Christian Couder @ 2023-10-10 12:48 UTC (permalink / raw)
To: Linus Arver
Cc: git, Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood, Calvin Wan,
Toon Claes, Christian Couder
In-Reply-To: <owlyledelnnd.fsf@fine.c.googlers.com>
On Sun, Sep 10, 2023 at 5:20 AM Linus Arver <linusa@google.com> wrote:
>
> Linus Arver <linusa@google.com> writes:
>
> >> +update refs/heads/branch1 ${NEW_branch1_HASH} ${OLD_branch1_HASH}
> >> +update refs/heads/branch2 ${NEW_branch2_HASH} ${OLD_branch2_HASH}
> >> +update refs/heads/branch3 ${NEW_branch3_HASH} ${OLD_branch3_HASH}
> >> +------------
> >> +
> >> +This will simultaneously rebase branch1, branch2, and branch3 -- all
> >> +commits they have since base, playing them on top of origin/main.
> >
> > How about
> >
> > This will rebase the commits in `branch1`, `branch2`, and `branch3`
> > (excluding those in `base`), preplaying them on top of `origin/main`.
>
> Oops, I meant "replaying" not "preplaying". But also, perhaps the
> following is simpler?
>
> This will replay the commits in `branch1`, `branch2`, and `branch3`
> (excluding those in `base`), on top of `origin/main`.
Here also I prefer to keep "rebase".
^ permalink raw reply
* Re: [PATCH v4 11/15] replay: use standard revision ranges
From: Christian Couder @ 2023-10-10 12:48 UTC (permalink / raw)
To: Linus Arver
Cc: git, Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood, Calvin Wan,
Toon Claes, Christian Couder
In-Reply-To: <owlyo7icl1g3.fsf@fine.c.googlers.com>
On Sat, Sep 9, 2023 at 12:55 AM Linus Arver <linusa@google.com> wrote:
>
> Hi Christian,
>
> I am only reviewing the docs. To assume the mindset of a Git user
> unfamiliar with this command, I purposely did not read the cover letter
> until after this review was done.
Ok, thanks!
> Christian Couder <christian.couder@gmail.com> writes:
>
> > [...]
> >
> > diff --git a/Documentation/git-replay.txt b/Documentation/git-replay.txt
> > new file mode 100644
> > index 0000000000..9a2087b01a
> > --- /dev/null
> > +++ b/Documentation/git-replay.txt
> > @@ -0,0 +1,90 @@
> > +git-replay(1)
> > +=============
> > +
> > +NAME
> > +----
> > +git-replay - Replay commits on a different base, without touching working tree
>
> How about using the same language ("new location") as in the DESCRIPTION
> heading?
I actually think that "base" is better than "location", also perhaps
saying things a bit differently than in the description can help
better understand the command.
I am Ok with replacing "different" with "new", mainly because it is
shorter, though.
> Also, the "without touching working tree" part is incomplete
> because as explained later on, the index and refs are also left alone.
I agree that it is incomplete.
> How about just "safely"?
I think that we want to say that the command can work on bare repos,
and I don't think "safely" conveys that meaning well. So now it is:
"git-replay - Replay commits on a new base, works on bare repos too"
> > +SYNOPSIS
> > +--------
> > +[verse]
> > +'git replay' --onto <newbase> <revision-range>...
> > +
> > +DESCRIPTION
> > +-----------
> > +
> > +Takes a range of commits, and replays them onto a new location.
>
> OK.
>
> > Does
> > +not touch the working tree or index, and does not update any
> > +references.
>
> How about this version?
>
> The new commits are created without modifying the working tree,
> index, or any references.
I agree it's nicer, but I prefer Dragan Simic's version.
> Also, by "references" we mean the refs in ".git/refs/*". In the
> gitrevisions man page we use the term "refnames" to refer to these bits,
> so maybe "refnames" is better than "references"? The simpler "branches"
> is another option.
"references" seems to be used much more often in the docs than
"refnames" (204 vs 18 occurrences). And deciding between "refnames"
and "references" in the docs is a global issue clearly outside the
scope of this series. So for now I will keep "references".
I don't like "branches" as I think tags and other refs are concerned too.
> > However, the output of this command is meant to be used
> > +as input to `git update-ref --stdin`, which would update the relevant
> > +branches.
>
> Before we get to this sentence, it would be great to explain why this
> command is useful (what problem does it solve)?
>
> Also, it would help to add "(see OUTPUT section below)" as a
> navigational aid in case some readers are wondering what the output
> looks like (and have not yet gotten to that section).
>
> I've noticed that you're using the phrase "starting point" in the
> OPTIONS section. I think this is better than "location" or "base" (FWIW
> we started using "starting point" in 0a02ca2383 (SubmittingPatches:
> simplify guidance for choosing a starting point, 2023-07-14)).
>
> The following is a version of this section that attempts to address my
> comments above (I've included the other bits already reviewed earlier to
> make it easier to read):
>
> Takes a range of commits, and replays them onto a new starting
> point. The new commits are created without modifying the working
> tree, index, or any branches. If there are branches that point to
> the commits in <revision-range>, list them in the output with both
> the original commit hash and the corresponding (replayed) commit
> hash (see OUTPUT section below) .
I like the "(see OUTPUT section below)" part, but I don't like the
fact that passing the output as input to `git update-ref --stdin`
isn't mentioned anymore.
So I have just added "(see the OUTPUT section below)".
> This command is like linkgit:git-rebase[1], but notably does not
> require a working tree or index.
I don't quite like this as the command will later also be like
cherry-pick (with the --advance mode).
> This means you can run this command
> in a bare repo (useful for server-side environments). And because
> nothing is modified (only new commits are created), it's like a "dry
> run" rebase.
Bare repos are now mentioned in the title of the page, so I am not
sure it's worth mentioning.
> By combining this command with `git update-ref --stdin`, the
> relevant branches can be updated. That is, the branches that were
> pointing originally to the commits given in the <revision-range>
> will be updated to point to the replayed commits. This is similar to
> the way how `git rebase --update-refs` updates multiple branches in
> the affected range.
I am not sure this is very useful, also I'm afraid that with all the
changes you propose the description section would be a bit too long.
Maybe you should propose some of the changes above in a follow up
patch series after this patch series has been merged.
> > +THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
> > +
> > +OPTIONS
> > +-------
> > +
> > +--onto <newbase>::
>
> How about 'starting-point' instead of 'newbase'?
I think "newbase" is very short and to the point. With --onto we
clearly want something that can do a rebase, so I think using
something with "base" in it is a good choice here.
> > + Starting point at which to create the new commits. May be any
> > + valid commit, and not just an existing branch name.
>
> Add "See linkgit:gitrevisions[7]." at the end?
I don't think it's worth it to mention "linkgit:gitrevisions[7]"
everywhere we can pass a revision. I think it makes the doc heavier
than it should be, especially here where the command is a plumbing one
for now and users should be quite experienced with Git already.
> > +The update-ref command(s) in the output will update the branch(es) in
> > +the revision range to point at the new commits, similar to the way how
> > +`git rebase --update-refs` updates multiple branches in the affected
> > +range.
>
> Ah, good example. I've moved this to my larger example above, so I don't
> think this paragraph is needed here any more (it probably didn't belong
> in OPTIONS anyway).
I think it's important here to help users understand that with this
option they have something very similar to "rebase".
> > +<revision-range>::
> > + Range of commits to replay; see "Specifying Ranges" in
> > + linkgit:git-rev-parse.
>
> OK.
>
> > +OUTPUT
> > +------
> > +
> > +When there are no conflicts, the output of this command is usable as
> > +input to `git update-ref --stdin`.
>
> What happens if there are conflicts? Probably good to mention in the
> DISCUSSION section. Some questions you may want to answer for the
> reader:
>
> (1) Is git-replay an all-or-nothing operation? That is, if there are any
> conflicts, is the output always empty (or do we still output those
> branches that *can* be updated without conflicts)?
>
> (2) What is meant by "conflict" for git-replay? Is it the same meaning
> as the case for git-rebase?
>
> For (1), in your cover letter under "# Important limitations" you say
> "No resumability" but I am not sure if this means git-replay will output
> *something* before exiting with an error, or simply nothing at all.
In a follow up series we might add options to generate some output in
case of conflicts. When we do that we will discuss this. So I think
for now it should be Ok to not talk more about the output in case of
conflicts.
> Speaking of the limitations section, perhaps it's worth pointing those
> out under DISCUSSION as well?
I don't think it's worth officially discussing the limitations in the
docs when some of them might be lifted soon after this series
graduates. The command is experimental, so I think users can
understand it if everything is not spelled out. Also if we spell out
things too much, users might rely on what we say when it could
actually change soon.
> > It is basically of the form:
>
> Why "basically"? Are there cases where the output can be different than
> the example given below? If not, then perhaps drop the word "basically"?
Ok, I have dropped "basically".
> > + update refs/heads/branch1 ${NEW_branch1_HASH} ${OLD_branch1_HASH}
> > + update refs/heads/branch2 ${NEW_branch2_HASH} ${OLD_branch2_HASH}
> > + update refs/heads/branch3 ${NEW_branch3_HASH} ${OLD_branch3_HASH}
> > +
> > +where the number of refs updated depends on the arguments passed and
> > +the shape of the history being replayed.
>
> Let's use "number of branches" instead of "number of refs" here to be
> consistent with the language elsewhere.
I prefer "refs" or "references" here.
> > +EXIT STATUS
> > +-----------
> > +
> > +For a successful, non-conflicted replay, the exit status is 0. When
> > +the replay has conflicts, the exit status is 1.
>
> OK.
>
> > If the replay is not
> > +able to complete (or start) due to some kind of error, the exit status
> > +is something other than 0 or 1.
>
> Not sure how useful "due to some kind of error" is here --- presumably
> the inability to replay is always due to some kind of error.
I think here "due to some kind of error" means something else than a
conflict which could also prevent the reply from fully "working".
> Would it be worth including a brief explanation about why git-replay
> might be unable to complete or start (is this behavior in practice a
> common-enough thing to document here)?
I don't think it's worth it at this step for this new command.
> > +EXAMPLES
> > +--------
> > +
> > +To simply rebase mybranch onto target:
>
> Looking at the CLI arguments, I think this phrase should be:
>
> Replay the commits in `origin/main..mybranch` onto `target`:
We suppose that users can understand that "rebase mybranch onto
target" means the same thing as what you suggest. Also I think it's
useful to associate "rebase" with "--onto" by deliberately using
"rebase" here.
However to make it a bit clearer, I agree that quoting `mybranch` and
`target` is better, so I have changed it to:
"To simply rebase `mybranch` onto `target`:"
> > +------------
> > +$ git replay --onto target origin/main..mybranch
> > +update refs/heads/mybranch ${NEW_mybranch_HASH} ${OLD_mybranch_HASH}
> > +------------
> > +
> > +When calling `git replay`, one does not need to specify a range of
> > +commits to replay using the syntax `A..B`; any range expression will
> > +do:
> > +
> > +------------
> > +$ git replay --onto origin/main ^base branch1 branch2 branch3
>
> Instead of `base`, how about `olderbranch`?
Sorry but like above, I like "base" here.
> > +update refs/heads/branch1 ${NEW_branch1_HASH} ${OLD_branch1_HASH}
> > +update refs/heads/branch2 ${NEW_branch2_HASH} ${OLD_branch2_HASH}
> > +update refs/heads/branch3 ${NEW_branch3_HASH} ${OLD_branch3_HASH}
> > +------------
> > +
> > +This will simultaneously rebase branch1, branch2, and branch3 -- all
> > +commits they have since base, playing them on top of origin/main.
>
> How about
>
> This will rebase the commits in `branch1`, `branch2`, and `branch3`
> (excluding those in `base`), preplaying them on top of `origin/main`.
>
> > +These three branches may have commits on top of base that they have in
> > +common, but that does not need to be the case.
>
> s/base/`base`
I am Ok with quoting `branch1`, `branch2`, `branch3`, `base` and
`origin/main`, but otherwise I prefer to keep the original wording.
> > +GIT
> > +---
> > +Part of the linkgit:git[1] suite
>
> One question I do have is what happens if you run git-replay twice
> (successfully)? Does the second invocation create another set of (new)
> successfully replayed commits?
Yes, I think it does that.
> I ask because I'm interested in informing
> the readers of our docs about any potential pitfalls from abusing this
> command by mistake.
I appreciate your desire to give high quality docs to our users, but I
don't think it's a big pitfall and I think that this command is still
very much "in the works" and is also designed for experienced users
for now, so I am not sure it's the right time to spend too much time
on this.
^ permalink raw reply
* [RFC] Define "precious" attribute and support it in `git clean`
From: Sebastian Thiel @ 2023-10-10 12:37 UTC (permalink / raw)
To: git; +Cc: Josh Triplett
[Note: I'm collaborating with Josh Triplett (CCed) on the design.]
I'd like to propose adding a new standard gitattribute "precious". I've
included proposed documentation at the end of this mail, and I'm happy to write
the code. I wanted to get feedback on the concept first.
What's a 'precious' file?
"Precious" files are files that are specific to a user or local configuration
and thus not tracked by Git. As such, a user spent time to create or generate
them, and to tune them to fit their needs. They are typically also ignored by
`git` due to `.gitignore` configuration, preventing them to be tracked by
accident.
This proposal suggests to make them known to Git using git-attributes so that
`git clean` can be taught to treat them with care.
Example: A Linux Kernel .config file
Users can mark the `.config` file as 'precious' using `.gitattributes`:
/.config precious
When checking which ignored files `git clean -nx` would remove, we would see
the following.
Would remove precious .config
Would remove scripts/basic/.fixdep.cmd
Would remove scripts/basic/fixdep
Would remove scripts/kconfig/.conf.cmd
This highlights precious files by calling them out, but doesn't change the
behaviour of existing flags. Instead, the new flag `-p` is added which lets
`git clean` spare precious files.
Thus `git clean -np` would print:
Would remove scripts/basic/.fixdep.cmd
Would remove scripts/basic/fixdep
Would remove scripts/kconfig/.conf.cmd
The precious file is not part of the set of files to be removed anymore.
`git clean -[n|f] -xp` will fail with an error indicating that `-x` and `-p`
are mutually exclusive. The hope is that people can replace some of their
usage of `-x` with `-p` to preserve precious files, while continuing to use
`-x` if they want a completely clean working directory.
Additional Benefits
`git clean -fdp` can now be used to restore the user's directory to a pristine
post-clone state while keeping all files and directories the project or user
identifies as precious. There is less fear of accidentally deleting files
which are required for local development or otherwise represent a time
investment.
Example: A precious IDE configuration directory.
To keep IDE configuration, one can also mark entire directories - the following
could go into a user-specific gitattributes file denoted by the
`core.attributesFile` configuration.
/.idea/** precious
With this attributes file in place, `git clean -ndx` would produce the
following output...
Would remove .DS_Store
Would remove precious .idea/
...while `git clean -ndp` would look like this:
Would remove .DS_Store
Here's a patch showing what the documentation could look like. Happy to write
the corresponding code.
---
diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 5e1a3d5148..5b2eab6573 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -60,6 +60,10 @@ OPTIONS
Use the given exclude pattern in addition to the standard ignore rules
(see linkgit:gitignore[5]).
+-p::
+ Remove ignored files as well (like `-x`), but preserve "precious"
+ files (see linkgit:gitattributes[5]).
+
-x::
Don't use the standard ignore rules (see linkgit:gitignore[5]), but
still use the ignore rules given with `-e` options from the command
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 6deb89a296..f68aadc3c2 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -1248,6 +1248,20 @@ If this attribute is not set or has an invalid value, the value of the
(See linkgit:git-config[1]).
+Preserving precious files
+~~~~~~~~~~~~~~~~~~~~~~~~~
+
+`precious`
+^^^^^^^^^^
+
+A file marked as `precious` will be preserved when running linkgit:git-clean[1]
+with the `-p` option. Use this attribute for files such as a Linux kernel
+`.config` file, which are not tracked by git because they contain user-specific
+or build-specific configuration, but which contain valuable information that a
+user spent time and effort to create.
+
+
+
USING MACRO ATTRIBUTES
----------------------
What do you think?
Thanks for your feedback,
Sebastian
^ permalink raw reply related
* Re: [PATCH v3 11/15] replay: use standard revision ranges
From: Christian Couder @ 2023-10-10 12:44 UTC (permalink / raw)
To: Dragan Simic
Cc: Toon Claes, git, Junio C Hamano, Patrick Steinhardt,
Johannes Schindelin, Elijah Newren, John Cai, Derrick Stolee,
Phillip Wood, Felipe Contreras, Calvin Wan, Christian Couder
In-Reply-To: <f412f62bf830b38a296b59ac3470099a@manjaro.org>
On Thu, Sep 7, 2023 at 11:02 PM Dragan Simic <dsimic@manjaro.org> wrote:
>
> On 2023-09-07 10:32, Christian Couder wrote:
> > On Thu, Jun 22, 2023 at 12:03 PM Toon Claes <toon@iotcl.com> wrote:
> >>
> >> Christian Couder <christian.couder@gmail.com> writes:
> >>
> >> > +DESCRIPTION
> >> > +-----------
> >> > +
> >> > +Takes a range of commits, and replays them onto a new location. Does
> >> > +not touch the working tree or index, and does not update any
> >> > +references. However, the output of this command is meant to be used
> >>
> >> Small suggestion here:
> >>
> >> Takes a range of commits, and replays them onto a new location. Does
> >> neither touch the working tree nor index, and does not update any
> >> references.
> >
> > I am not a native speaker, so I am not sure what's best here. I find
> > your suggestion a bit less clear though, so until a native speaker
> > agrees with it or maybe finds something even better, I prefer to leave
> > it as-is.
>
> I'm also not a native English speaker, but I spent about 2.5 years
> contributing a whole lot to English Wikipedia, so I'd dare to say I've
> honed my English skills rather well. Thus, here's my take on this:
>
> Takes a range of commits and replays them onto a new location.
> Leaves the working tree and the index untouched, and updates no
> references. The output of this command is to be used...
>
> This is written in a concise yet slightly imperative way, which should
> be suitable for the purpose. I hope you agree.
I agree and I like it, so I have changed it to the above in version 5
I just sent.
Thanks!
^ permalink raw reply
* Re: [PATCH v4 06/15] replay: don't simplify history
From: Christian Couder @ 2023-10-10 12:43 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Christian Couder
In-Reply-To: <58daa706-7efb-51dd-9061-202ef650b96a@gmx.de>
Hi Dscho,
On Thu, Sep 7, 2023 at 1:02 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Christian & Elijah,
>
> On Thu, 7 Sep 2023, Christian Couder wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > Let's set the rev walking options we need after calling
> > setup_revisions() instead of before. This makes it clearer which options
> > we need.
>
> In light of the currently open issue about command-line validation, this
> change does more than this paragraph lets on: It hardcodes certain
> settings, overriding (silently) any rev-list options the user might have
> passed.
>
> Is there any chance that we can avoid this change?
In the version 5 I just sent, I have changed the commit message and the code.
The commit messages says:
"
replay: change rev walking options
Let's set the rev walking options we need after calling
setup_revisions() instead of before. This enforces options we always
want.
We want the command to work from older commits to newer ones by default,
but we are Ok with letting users reverse that, using --reverse, if that's
what they really want.
Also we don't want history simplification, as we want to deal with all
the commits in the affected range.
"
So about "revs.reverse" the code is now:
revs.reverse = !revs.reverse;
which seems to be what you suggested elsewhere.
Apart from that I think it's fair to enforce some values for a few
options. This way we can make the command work the way we want by
default, get consistent behavior and avoid users shooting themselves
in the foot for now. If more flexibility is needed and useful in the
future, then we might allow it in future patches with proper
justification, tests and docs. There is still a lot of flexibility
left especially as the patch that disallowed some rev walking options
and pathspecs has been removed as you suggested.
> > Also we don't want history simplification, as we want to deal with all
> > the commits in the affected range.
>
> This, however, is a good change. It deserves to live in its own commit,
> with its own commit message, in particular because it is not obvious from
> the attribute names which ones we're talking about (I guess it's `limited`
> and `simplify_history`, not just the latter.
We only enforce "revs.sort_order", "revs.topo_order" and yeah
"revs.simplify_history".
Also I don't think it makes much sense to separate these changes in
different commits/patches. I am Ok to improve the commit message if
you think it's worth it, but the patch series is designed to make it
easy to review the changes from the "fast-rebase" test-tool to a good
"replay" plumbing command, and I don't think separating those related
changes would improve things much.
^ permalink raw reply
* Re: [PATCH v4 02/15] replay: introduce new builtin
From: Christian Couder @ 2023-10-10 12:42 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Christian Couder
In-Reply-To: <52277471-4ddd-b2e0-62ca-c2a5b59ae418@gmx.de>
Hi Dscho,
On Thu, Sep 7, 2023 at 1:01 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Christian & Elijah,
>
> On Thu, 7 Sep 2023, Christian Couder wrote:
>
> > diff --git a/command-list.txt b/command-list.txt
> > index 54b2a50f5f..d74836ab21 100644
> > --- a/command-list.txt
> > +++ b/command-list.txt
> > @@ -160,6 +160,7 @@ git-reflog ancillarymanipulators complete
> > git-remote ancillarymanipulators complete
> > git-repack ancillarymanipulators complete
> > git-replace ancillarymanipulators complete
> > +git-replay mainporcelain history
>
> I recall this having come up before, but in light of the still active
> discussion revolving around command-line parameter validation, I would
> strongly advise removing `git replay` from the main page and instead go
> for plumbingmanipulators.
Ok, I have done that in the version 5 I just sent.
^ permalink raw reply
* [PATCH v5 14/14] replay: stop assuming replayed branches do not diverge
From: Christian Couder @ 2023-10-10 12:38 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood, Calvin Wan,
Toon Claes, Dragan Simic, Linus Arver, Christian Couder
In-Reply-To: <20231010123847.2777056-1-christian.couder@gmail.com>
From: Elijah Newren <newren@gmail.com>
The replay command is able to replay multiple branches but when some of
them are based on other replayed branches, their commit should be
replayed onto already replayed commits.
For this purpose, let's store the replayed commit and its original
commit in a key value store, so that we can easily find and reuse a
replayed commit instead of the original one.
Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin/replay.c | 44 ++++++++++++++++++++++++++--------
t/t3650-replay-basics.sh | 52 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 86 insertions(+), 10 deletions(-)
diff --git a/builtin/replay.c b/builtin/replay.c
index 12689f1c89..f74261b75e 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -223,20 +223,33 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
strset_clear(&rinfo.positive_refs);
}
+static struct commit *mapped_commit(kh_oid_map_t *replayed_commits,
+ struct commit *commit,
+ struct commit *fallback)
+{
+ khint_t pos = kh_get_oid_map(replayed_commits, commit->object.oid);
+ if (pos == kh_end(replayed_commits))
+ return fallback;
+ return kh_value(replayed_commits, pos);
+}
+
static struct commit *pick_regular_commit(struct commit *pickme,
- struct commit *last_commit,
+ kh_oid_map_t *replayed_commits,
+ struct commit *onto,
struct merge_options *merge_opt,
struct merge_result *result)
{
- struct commit *base;
+ struct commit *base, *replayed_base;
struct tree *pickme_tree, *base_tree;
base = pickme->parents->item;
+ replayed_base = mapped_commit(replayed_commits, base, onto);
+ result->tree = repo_get_commit_tree(the_repository, replayed_base);
pickme_tree = repo_get_commit_tree(the_repository, pickme);
base_tree = repo_get_commit_tree(the_repository, base);
- merge_opt->branch1 = short_commit_name(last_commit);
+ merge_opt->branch1 = short_commit_name(replayed_base);
merge_opt->branch2 = short_commit_name(pickme);
merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
@@ -250,7 +263,7 @@ static struct commit *pick_regular_commit(struct commit *pickme,
merge_opt->ancestor = NULL;
if (!result->clean)
return NULL;
- return create_commit(result->tree, pickme, last_commit);
+ return create_commit(result->tree, pickme, replayed_base);
}
int cmd_replay(int argc, const char **argv, const char *prefix)
@@ -266,6 +279,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
struct merge_options merge_opt;
struct merge_result result;
struct strset *update_refs = NULL;
+ kh_oid_map_t *replayed_commits;
int ret = 0;
const char * const replay_usage[] = {
@@ -324,21 +338,30 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
init_merge_options(&merge_opt, the_repository);
memset(&result, 0, sizeof(result));
merge_opt.show_rename_progress = 0;
-
- result.tree = repo_get_commit_tree(the_repository, onto);
last_commit = onto;
+ replayed_commits = kh_init_oid_map();
while ((commit = get_revision(&revs))) {
const struct name_decoration *decoration;
+ khint_t pos;
+ int hr;
if (!commit->parents)
die(_("replaying down to root commit is not supported yet!"));
if (commit->parents->next)
die(_("replaying merge commits is not supported yet!"));
- last_commit = pick_regular_commit(commit, last_commit, &merge_opt, &result);
+ last_commit = pick_regular_commit(commit, replayed_commits, onto,
+ &merge_opt, &result);
if (!last_commit)
break;
+ /* Record commit -> last_commit mapping */
+ pos = kh_put_oid_map(replayed_commits, commit->object.oid, &hr);
+ if (hr == 0)
+ BUG("Duplicate rewritten commit: %s\n",
+ oid_to_hex(&commit->object.oid));
+ kh_value(replayed_commits, pos) = last_commit;
+
/* Update any necessary branches */
if (advance_name)
continue;
@@ -367,13 +390,14 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
}
merge_finalize(&merge_opt, &result);
- ret = result.clean;
-
-cleanup:
+ kh_destroy_oid_map(replayed_commits);
if (update_refs) {
strset_clear(update_refs);
free(update_refs);
}
+ ret = result.clean;
+
+cleanup:
release_revisions(&revs);
/* Return */
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index d6286f9580..389670262e 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -143,4 +143,56 @@ test_expect_success 'using replay on bare repo to also rebase a contained branch
test_cmp expect result-bare
'
+test_expect_success 'using replay to rebase multiple divergent branches' '
+ git replay --onto main ^topic1 topic2 topic4 >result &&
+
+ test_line_count = 2 result &&
+ cut -f 3 -d " " result >new-branch-tips &&
+
+ git log --format=%s $(head -n 1 new-branch-tips) >actual &&
+ test_write_lines E D M L B A >expect &&
+ test_cmp expect actual &&
+
+ git log --format=%s $(tail -n 1 new-branch-tips) >actual &&
+ test_write_lines J I M L B A >expect &&
+ test_cmp expect actual &&
+
+ printf "update refs/heads/topic2 " >expect &&
+ printf "%s " $(head -n 1 new-branch-tips) >>expect &&
+ git rev-parse topic2 >>expect &&
+ printf "update refs/heads/topic4 " >>expect &&
+ printf "%s " $(tail -n 1 new-branch-tips) >>expect &&
+ git rev-parse topic4 >>expect &&
+
+ test_cmp expect result
+'
+
+test_expect_success 'using replay on bare repo to rebase multiple divergent branches, including contained ones' '
+ git -C bare replay --contained --onto main ^main topic2 topic3 topic4 >result &&
+
+ test_line_count = 4 result &&
+ cut -f 3 -d " " result >new-branch-tips &&
+
+ >expect &&
+ for i in 2 1 3 4
+ do
+ printf "update refs/heads/topic$i " >>expect &&
+ printf "%s " $(grep topic$i result | cut -f 3 -d " ") >>expect &&
+ git -C bare rev-parse topic$i >>expect || return 1
+ done &&
+
+ test_cmp expect result &&
+
+ test_write_lines F C M L B A >expect1 &&
+ test_write_lines E D C M L B A >expect2 &&
+ test_write_lines H G F C M L B A >expect3 &&
+ test_write_lines J I M L B A >expect4 &&
+
+ for i in 1 2 3 4
+ do
+ git -C bare log --format=%s $(grep topic$i result | cut -f 3 -d " ") >actual &&
+ test_cmp expect$i actual || return 1
+ done
+'
+
test_done
--
2.42.0.339.g663cbc8ab1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox