Re: [PATCH v2] read-cache: write all indexes with the same permissions
Ævar Arnfjörð Bjarmason writes: > Do you mean that you don't agree that following should always create > both "foo" and e.g. ".git/refs/heads/master" with the same 644 > (-rw-rw-r--) mode: > > ( > rm -rf /tmp/repo && > umask 022 && > git init /tmp/repo && > cd /tmp/repo && > echo hi >foo && > git add foo && > git commit -m"first" > ) > > To me what we should do with the standard umask and what > core.sharedRepository are for are completely different things. Ahh, of course. If you put it that way, I do agree that it gives us a valid use case where core.sharedRepository is false and the umask of repository owner is set to 022 (or anything that does not allow write to group or others, and allows read to group) to let group members only peek but not touch the contents of the repository. I think I was distracted by the mention of ore.sharedRepository in the proposed log message.
Re: [PATCH v3 0/1] Fix scissors bug during merge conflict
Denton Liu writes:
>> I wonder if this is what you really meant to have instead:
>>
>> >else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
>> > - whence == FROM_COMMIT)
>> > - wt_status_add_cut_line(s->fp);
>> > + whence == FROM_COMMIT) {
>> > + if (!merge_contains_scissors)
>> > + wt_status_add_cut_line(s->fp);
>> > + }
>> >else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
>> >status_printf(s, GIT_COLOR_NORMAL,
>>
>> That is, the only behaviour change in "merge contains scissors"
>> mode is to omit the cut line and nothing else.
>
> Thanks for catching this. I noticed that the originally behaviour is
> buggy too: in the case where we're merging a commit and scissors are
> printed from the `if (whence != FROM_COMMIT)` block, the original
> behaviour would drop us into the else (COMMIT_MSG_CLEANUP_SPACE)
> statement, which is undesired.
The original calls add-cut-line in the "whence != FROM_COMMIT" when
cleanup_mode is CLEANUP_SCISSORS (and then in that block it also adds
the message about committing a merge or cherry-pick). After that,
the original does the three-arm if/else if/else cascade and we end
up showing the "lines starting with # will be kept" message.
Yeah, you read the original correctly and I agree that in that block
the right thing to do is not to say anything in CLEANUP_SCISSORS
mode.
Thanks for carefully reading the code again.
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Slavica Djukic writes: >> Yes, but for that you'd need to be checking the resulting commit >> object that represents the stash entry. It should be created under >> the substitute identity. > Would it be correct to check it like this: > > git reset && > >1 && > git add 1 && > echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>" >expect && > git stash && > git show -s --format="%an <%ae>" refs/stash >actual && > test_cmp expect actual So, you create a stash, and grab %an and %ae out of the resulting commit object and store them in actual, and then compare. Makes sense.
Re: [PATCH] technical doc: add a design doc for the evolve command
Resending this without HTML enabled... sorry if you receive it twice. > The file name and the title are in a mismatch. Good point. However, the focus of this proposal really is supposed to be on the underlying data structure, not just the evolve command (which is the driving use-case for the graph but not the only important one). I think I'll fix the mismatch by renaming both the title and document to "change graph" if that seems acceptable. I'll also expand the "objective" paragraph to mention the evolve command. > Perhaps"three sequential patches"? I've added a quick informal definition of the word "change", along with a cross-reference to the precise definition later in the document. > These two paragraphs could be moved lower, under a "Semi-Related Work" Good point. I'll keep the patch queue managers here since they really can be used to solve the same problem that evolve addresses, but I'll move replacements paragraph down to a new section on semi-related work. There was also a request to discuss git-imerge which I'll insert there. > Instead, I would try to use the term "patch" to describe a change to the > codebase I know you didn't finish the document but later on I define the term "change" to have essentially this meaning. I've moved the definition earlier in the document to make the earlier sections easier to understand. Given the choice of the word "patch" or "change" for this definition, I prefer to use "change" since gerrit already defines it in this way and the word "patch" already has a meaning in git (a file containing a diff). > Making a note so I come back to this. I hope to learn what you mean by this > "more specific merge base".) Lets say we have commits: P <- C Then two people amend C in different ways producing: P <- C P <- C1 P <- C2 ...then we try to resolve the divergence by merging C1 and C2. Without the change graph, the closest merge-base (ancestor) would be P. With the change graph, the closest merge base would be C. > If we GC'd commit A but still have the newer A', I can either thinkthat I'm not sure I followed that. Are you suggesting a change to the proposal or asking for a clarification? On Fri, Nov 16, 2018 at 1:36 PM Derrick Stolee wrote: > > On 11/14/2018 7:55 PM, sxe...@google.com wrote: > > From: Stefan Xenos > > > > This document describes what an obsolescence graph for > > git would look like, the behavior of the evolve command, > > and the changes planned for other commands. > > Thanks for putting this together! > > > diff --git a/Documentation/technical/evolve.txt > > b/Documentation/technical/evolve.txt > ... > > +Git Obsolescence Graph > > +== > > + > > +Objective > > +- > > +Track the edits to a commit over time in an obsolescence graph. > > The file name and the title are in a mismatch. > > I'd prefer if the title was "Git Evolve Design Document" and this > opening paragraph > was about the reasons we want a 'git evolve' command. Here is my attempt: > >The proposed 'git evolve' command will help users craft a > high-quality commit >history in their topic branches. By working to improve commits one at > a time, >then running 'git evolve', users can rewrite recent history with more > options >than interactive rebase. The core benefit is that users can pause > their progress >and move to other branches before returning to where they left off. > Users can >also share progress with others using standard 'push', 'fetch', and > 'format-patch' >commands. > > > +Background > > +-- > > Perhaps you can call this "Example"? > > > +Imagine you have three dependent changes up for review and you receive > > feedback > > +that requires editing all three changes. While you're editing one, more > > feedback > > +arrives on one of the others. What do you do? > > "three dependent changes" sounds a bit vague enough to me to possibly > confuse readers. Perhaps > "three sequential patches"? > > > +- Users can view the history of a commit directly (the sequence of amends > > and > > + rebases it has undergone, orthogonal to the history of the branch it is > > on). > > "the history of a commit" doesn't semantically work, as a commit is an > immutable Git object. > > Instead, I would try to use the term "patch" to describe a change to the > codebase, and that > takes the form as a list of commits that are improving on each other > (but don't actually > have each other in their commit history). This means that the lifetime > of a patch is described > by the commits that are amended or rebased. > > > +- By pushing and pulling the obsolescence graph, users can collaborate more > > + easily on changes-in-progress. This is better than pushing and pulling > > the > > + changes themselves since the obsolescence graph can be used to locate a > > more > > + specific merge base, allowing for better merges between different > > versions of > > + the same change. > > (Making a note so I come back to
[PATCH v3 1/1] merge: add scissors line on merge conflict
This fixes a bug where the scissors line is placed after the Conflicts:
section, in the case where a merge conflict occurs and
merge.cleanup = scissors.
Next, if commit.cleanup = scissors is specified, don't produce a
scissors line in commit if one already exists in the MERGE_MSG file.
Finally, we give pull the passthrough option of --cleanup so that it
can also take advantage of this change.
Signed-off-by: Denton Liu
---
Documentation/merge-options.txt | 6 +
builtin/commit.c| 20 ++
builtin/merge.c | 16 +++
builtin/pull.c | 6 +
t/t7600-merge.sh| 48 +
5 files changed, 91 insertions(+), 5 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 63a3fc0954..115e0ca6f0 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -27,6 +27,12 @@ they run `git merge`. To make it easier to adjust such
scripts to the
updated behaviour, the environment variable `GIT_MERGE_AUTOEDIT` can be
set to `no` at the beginning of them.
+--cleanup=::
+ This option determines how the merge message will be cleaned up
+ before being passed on. Specifically, if the '' is given a
+ value of `scissors`, scissors will be prepended to the message in
+ the case of a merge conflict. See also linkgit:git-commit[1].
+
--ff::
When the merge resolves as a fast-forward, only update the branch
pointer, without creating a merge commit. This is the default
diff --git a/builtin/commit.c b/builtin/commit.c
index 0d9828e29e..7902645bc9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -659,6 +659,7 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
const char *hook_arg2 = NULL;
int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
int old_display_comment_prefix;
+ int merge_contains_scissors = 0;
/* This checks and barfs if author is badly specified */
determine_author_info(author_ident);
@@ -719,6 +720,8 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
strbuf_addbuf(, );
hook_arg1 = "message";
} else if (!stat(git_path_merge_msg(the_repository), )) {
+ size_t merge_msg_start;
+
/*
* prepend SQUASH_MSG here if it exists and a
* "merge --squash" was originally performed
@@ -729,8 +732,14 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
hook_arg1 = "squash";
} else
hook_arg1 = "merge";
+
+ merge_msg_start = sb.len;
if (strbuf_read_file(, git_path_merge_msg(the_repository),
0) < 0)
die_errno(_("could not read MERGE_MSG"));
+
+ if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
+ wt_status_locate_end(sb.buf + merge_msg_start, sb.len -
merge_msg_start) < sb.len - merge_msg_start)
+ merge_contains_scissors = 1;
} else if (!stat(git_path_squash_msg(the_repository), )) {
if (strbuf_read_file(, git_path_squash_msg(the_repository),
0) < 0)
die_errno(_("could not read SQUASH_MSG"));
@@ -798,7 +807,8 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
struct ident_split ci, ai;
if (whence != FROM_COMMIT) {
- if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
+ if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
+ !merge_contains_scissors)
wt_status_add_cut_line(s->fp);
status_printf_ln(s, GIT_COLOR_NORMAL,
whence == FROM_MERGE
@@ -823,10 +833,10 @@ static int prepare_to_commit(const char *index_file,
const char *prefix,
_("Please enter the commit message for your
changes."
" Lines starting\nwith '%c' will be ignored,
and an empty"
" message aborts the commit.\n"),
comment_line_char);
- else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
-whence == FROM_COMMIT)
- wt_status_add_cut_line(s->fp);
- else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
+ else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
+ if (whence == FROM_COMMIT && !merge_contains_scissors)
+ wt_status_add_cut_line(s->fp);
+ } else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
status_printf(s, GIT_COLOR_NORMAL,
_("Please enter the commit message for
[PATCH v3 0/1] Fix scissors bug during merge conflict
On Sat, Nov 17, 2018 at 05:06:43PM +0900, Junio C Hamano wrote:
> Are we already sometimes adding a scissors line in that file? The
> impression I was getting was that the change in the step 2/2 is the
> only reason why this becomes necessary. In which case this change
> makes no sense as a standalone patch and requires 2/2 to be a
> sensible change, no?
>
My mistake, I guess I went a little overboard trying to split my
contribution into digestable patches.
> > @@ -798,7 +807,8 @@ static int prepare_to_commit(const char *index_file,
> > const char *prefix,
> > struct ident_split ci, ai;
> >
> > if (whence != FROM_COMMIT) {
> > - if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
> > + if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
> > + !merge_contains_scissors)
> > wt_status_add_cut_line(s->fp);
> > status_printf_ln(s, GIT_COLOR_NORMAL,
> > whence == FROM_MERGE
>
> This one is done before we show a block of text, which looks correct.
>
> > @@ -824,7 +834,8 @@ static int prepare_to_commit(const char *index_file,
> > const char *prefix,
> > " Lines starting\nwith '%c' will be ignored,
> > and an empty"
> > " message aborts the commit.\n"),
> > comment_line_char);
> > else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
> > -whence == FROM_COMMIT)
> > +whence == FROM_COMMIT &&
> > +!merge_contains_scissors)
> > wt_status_add_cut_line(s->fp);
> > else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> > status_printf(s, GIT_COLOR_NORMAL,
>
> The correctness of this one in an if/elseif/else cascade is less
> clear. When "contains scissors" logic does not kick in, we just
> have a scissors line here (i.e. the original behaviour). Now when
> the new logic kicks in, we not just omit the (extra) scissors line,
> but also say "Please enter the commit message..." which is the
> message used with the "comment line char" mode of the clean-up.
>
> I wonder if this is what you really meant to have instead:
>
> > else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
> > -whence == FROM_COMMIT)
> > - wt_status_add_cut_line(s->fp);
> > +whence == FROM_COMMIT) {
> > +if (!merge_contains_scissors)
> > + wt_status_add_cut_line(s->fp);
> > + }
> > else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> > status_printf(s, GIT_COLOR_NORMAL,
>
> That is, the only behaviour change in "merge contains scissors"
> mode is to omit the cut line and nothing else.
Thanks for catching this. I noticed that the originally behaviour is
buggy too: in the case where we're merging a commit and scissors are
printed from the `if (whence != FROM_COMMIT)` block, the original
behaviour would drop us into the else (COMMIT_MSG_CLEANUP_SPACE)
statement, which is undesired. With this fix, the message about `#`
_not_ being removed is now silenced in both cases.
Changes since V1:
* Only check MERGE_MSG for a scissors line instead of all prepended
files
* Make a variable static in merge where appropriate
* Add passthrough options in pull
* Add documentation for the new option
* Add tests to ensure desired behaviour
Changes since V2:
* Merge both patches into one patch
* Fix bug in help message printing logic
Denton Liu (1):
merge: add scissors line on merge conflict
Documentation/merge-options.txt | 6 +
builtin/commit.c| 20 ++
builtin/merge.c | 16 +++
builtin/pull.c | 6 +
t/t7600-merge.sh| 48 +
5 files changed, 91 insertions(+), 5 deletions(-)
--
2.19.1
Re: [PATCH v2] read-cache: write all indexes with the same permissions
On Sat, Nov 17 2018, Junio C Hamano wrote:
> Christian Couder writes:
>
>> "However, as noted in those commits we'd still create the file as
>> 0600, and would just re-chmod it only if core.sharedRepository is set
>> to "true" or "all". If core.sharedRepository is unset or set to
>> "false", then the file mode will not be changed, so without
>> core.splitIndex a system with e.g. the umask set to group writeability
>> would work for a group member, but not with core.splitIndex set, as
>> group members would not be able to access the shared index file.
>
> That is irrelevant. The repository needs to be configured properly
> if it wanted to be used by the members of the group, period.
>
>> It is unfortunately not short lived when core.sharedrepository is
>> unset for example as adjust_shared_perm() starts with:
>>
>> int adjust_shared_perm(const char *path)
>> {
>> int old_mode, new_mode;
>>
>> if (!get_shared_repository())
>> return 0;
>>
>> but get_shared_repository() will return PERM_UMASK which is 0 when
>> git_config_get_value("core.sharedrepository", ...) returns a non zero
>> value which happens when "core.sharedrepository" is unset.
>
> Which is to say, you get an unwanted result when your repository is
> not configured properly. It is not a news, and I have no sympathy.
>
> Just configure your repository properly and you'll be fine.
>
>>> > Ideally we'd split up the adjust_shared_perm() function to one that
>>> > can give us the mode we want so we could just call open() instead of
>>> > open() followed by chmod(), but that's an unrelated cleanup.
>>>
>>> I would drop this paragraph, as I think this is totally incorrect.
>>> Imagine your umask is tighter than the target permission. You ask
>>> such a helper function and get "you want 0660". Doing open(0660)
>>> would not help you an iota---you'd need chmod() or fchmod() to
>>> adjust the result anyway, which already is done by
>>> adjust-shared-perm.
>>
>> It seems to me that it is not done when "core.sharedrepository" is unset.
>
> So? You are assuming that the repository is misconfigured and it is
> not set to widen the perm bit in the first place, no?
>
>>> > We already have that minor issue with the "index" file
>>> > #leftoverbits.
>>>
>>> The above "Ideally", which I suspect is totally bogus, would show up
>>> whey people look for that keyword in the list archive. This is one
>>> of the reasons why I try to write it after at least one person
>>> sanity checks that an idea floated is worth remembering.
>>
>> It was in Ævar's commit message and I thought it might be better to
>> keep it so that people looking for that keyword could find the above
>> as well as the previous RFC patch.
>
> So do you agree that open(0660) does not guarantee the result will
> be group writable, the above "Ideally" is misguided nonsense, and
> giving the #leftoverbits label to it will clutter the search result
> and harm readers? That's good.
Aside from issues with the clarity of the commit message, which I'll fix
& thanks for pointing them out. I think we may have stumbled on
something more important here.
Do you mean that you don't agree that following should always create
both "foo" and e.g. ".git/refs/heads/master" with the same 644
(-rw-rw-r--) mode:
(
rm -rf /tmp/repo &&
umask 022 &&
git init /tmp/repo &&
cd /tmp/repo &&
echo hi >foo &&
git add foo &&
git commit -m"first"
)
To me what we should do with the standard umask and what
core.sharedRepository are for are completely different things.
We should in git be creating files such that if I set my umask to
e.g. 022 all users on the system can read what I'm creating.
E.g. I tend to use this on something like a production server where
others (if I'm asleep) might want to look at my .bash_history as a last
resort, and also some one-off repo I've created without setting
core.sharedRepository.
I've yet to run into a case where this doesn't just work, aside from
core.splitIndex where before the patch here we're using a tempfile API
for something that isn't a tempfile.
This is distinct from the core.sharedRepository use-case, where you'd
like to on a per-repo basis override what you'd otherwise get with the
umask. E.g. if you have a shared server hosting a shared git repo, where
users with umask 077 will still be forced to create e.g. group rw files.
Re: [PATCH] technical doc: add a design doc for the evolve command
> I am not sure that we necessarily need this to be a graph. I think part > of the problems with not being able to GC *any* of this is by this > requirement to have it stored in a graph, rather than having mappings from > which you could reconstruct any non-GC'ed parts of that graph, if you > really want. Sorry, I'm not sure what GC problem you're alluding to here. As far as I'm aware, this proposal should permit us to GC or retain any subset of commits that we want. We create a chain of metacommits pointing to the commits we want to retain, and put a ref in the metas namespace to cause the chain itself to be retained. If we want to GC a different subset, we can build a different chain of metacommits and move the ref (or delete the ref entirely to permit the whole chain to be gc'd). Could you be more specific about which use-case is problematic? > Why is this missing most notably `hg evolve`? Good point. I'll add a brief description and comparison to the doc. > Also, please do not forget `git imerge`. Thanks for directing me to this. It looks fantastic! I'm not sure it's really an alternative to this work, but I could see adding an argument to "git evolve" that allows you to use imerge for resolving merge conflicts at any given step. > Further, I see that this document tries to suggest a proliferation of new > commands It does. Let me explain a bit about the reasoning behind this breakdown of commands. My main priority was to keep the commands as consistent with existing git commands as possible. Secondary goals were: - Mapping a single intent to a single command where possible makes it easier to explain what that command does. - Having lots of simpler commands as opposed to a few complex commands makes them easier to type. - Command names are more descriptive than lettered arguments. Git already has a "log" and "reflog" command for displaying two different types of log, so putting "obslog" on its own command makes it consistent with the existing logs, easier to type, and keeps the command simple. The "evolve" command updates changes to give them up-to-date parents. This is a new type of user intent that didn't exist previously in git, so putting it on its own command keeps things simpler for users. The relationship between the evolve and change commands is a lot like the the relationship between the rebase command and the branch commands. They could technically be combined into one command but I'm not sure this would help with usability. The "change" command combines many user intents (create a change, rename a change, delete a change, etc.) If I were to design it from scratch, I'd prefer to have all of these things on separate commands. However, since changes are very similar to branches and users are presumably already familiar with the branch command, I intentionally made the change command as close as possible to the branch command - using the same arguments for the same purpose. In this case, I sacrificed the single-intent and simple commands goals in order to retain consistency. Anyway, that was my reasoning behind the selection of commands. Of course, I'd welcome feedback - a good UX is the one that was built by listening to feedback from its intended users. Personally, I don't consider a proliferation of new commands to be inherently bad (or inherently good, really). Is there a reason new commands should be avoided? Some other alternatives to consider: - We could turn "obslog" into an extra option on the "log" command, but that would be inconsistent with reflog and would complicate the already-complex log command. - If we were to combine "evolve" with another command, "git rebase --evolve" would probably be the best candidate. However, this is longer to type and I tend to prefer lots of simple commands over a few complex ones. Also, the evolve command will get additional options in the future (to enable stuff like amend-over-cherry-pick, various automatic resolution strategies for divergence, etc.)... and putting it on rebase would mean we'd end up with a lot of extra arguments whose doc says "this argument is only used if you're also using --evolve". - We could break the "change" command into a bunch of simpler ones "lschange", "mkchange", "rmchange", "mvchange", etc. I actually like this a lot, but this would make it diverge from the "branch" command so I'm not sure we should do it unless enough of us feel the same way. - We could combine the "change" command with the "branch" command. The branch command could look for the "metas" prefix to determine whether its argument is a branch or a change -- or it could just search one namespace followed by the other. This would make for fewer commands, but I'm concerned it may create confusion by making changes resemble branches too closely. If you're not already familiar with the distinction, you may see unexpected behavior when the "branch" you think you're manipulating turns out to be a change. - Stefan On Thu, Nov 15, 2018 at 4:52
From Michelle
Bitte bestätigen Sie meinen Vorschlag, es ist sehr dringend, ich habe Ihr Profil gesehen und bin es wirklich interessiert an dir, bitte antworte Michelle
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Hi Junio, On 16-Nov-18 11:12 AM, Junio C Hamano wrote: Slavica Djukic writes: + git var GIT_COMMITTER_IDENT >actual && + test_cmp expected actual && I am not sure what you are testing with this step. There is nothing that changed environment variables or configuration since we ran "git var" above. Why does this test suspect that somebody in the future may break the expectation that after running 'git add' and/or 'git stash', our committer identity may have been changed, and how would such a breakage happen? In previous re-roll, you suggested that test should be improved so that when reasonable identity is present, git stash executes under that identity, and not under the fallback one. Yes, but for that you'd need to be checking the resulting commit object that represents the stash entry. It should be created under the substitute identity. Would it be correct to check it like this: git reset && >1 && git add 1 && echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>" >expect && git stash && git show -s --format="%an <%ae>" refs/stash >actual && test_cmp expect actual It is similar to your suggestion when there is no ident present. Here I'm just making sure that after calling git stash, we still have same reasonable identity present. I do not think such a test would detect it, even when "git stash" incorrectly used the fallback identity to create the stash entry. Thank you, Slavica
[PATCH/RFC v1 1/1] Use size_t instead of unsigned long
From: Torsten Bögershausen
Currently Git users can not commit files >4Gib under 64 bit Windows,
where "long" is 32 bit but size_t is 64 bit.
Improve the code base in small steps, as small as possible.
What started with a small patch to replace "unsigned long" with size_t
in one file (convert.c) ended up with a change in many files.
Signed-off-by: Torsten Bögershausen
---
This needs to go on top of pu, to cover all the good stuff
cooking here.
I have started this series on November 1st, since that 2 or 3 rebases
had been done to catch up, and now it is on pu from November 15.
I couldn't find a reason why changing "unsigned ling"
into "size_t" may break anything, any thoughts, please ?
Side question: One thing I wondered about is why Git creates a conflict
like this, using git cherry-pick:
<<< HEAD
unsigned long size;
void *data = read_object_file(oid, , );
===
size_t size;
void *data = repo_read_object_file(the_repository, oid, ,
);
>>> 3ee0abef4c... Use size_t instead of unsigned long
One commit changed "unsigned long size" into "size_t size",
the other commit swapped repo_read_object_file() with read_object_file().
Both changed are on different lines, but Git sees a conflict here.
apply.c | 14 -
archive-tar.c| 18 +--
archive-zip.c| 2 +-
archive.c| 2 +-
archive.h| 2 +-
bisect.c | 2 +-
blame.c | 6 ++--
blame.h | 2 +-
builtin/cat-file.c | 10 +++---
builtin/difftool.c | 3 +-
builtin/fast-export.c| 6 ++--
builtin/fmt-merge-msg.c | 4 ++-
builtin/fsck.c | 6 ++--
builtin/grep.c | 8 ++---
builtin/index-pack.c | 27
builtin/log.c| 4 +--
builtin/ls-tree.c| 2 +-
builtin/merge-tree.c | 6 ++--
builtin/mktag.c | 5 +--
builtin/notes.c | 6 ++--
builtin/pack-objects.c | 56 +-
builtin/reflog.c | 2 +-
builtin/replace.c| 2 +-
builtin/tag.c| 4 +--
builtin/unpack-file.c| 2 +-
builtin/unpack-objects.c | 35 ++---
builtin/verify-commit.c | 4 +--
bundle.c | 2 +-
cache.h | 10 +++---
combine-diff.c | 11 ---
commit.c | 22 +++---
commit.h | 10 +++---
config.c | 2 +-
convert.c| 18 +--
delta.h | 20 ++--
diff-delta.c | 4 +--
diff.c | 30 +-
diff.h | 2 +-
diffcore-pickaxe.c | 4 +--
diffcore.h | 2 +-
dir.c| 6 ++--
dir.h| 2 +-
entry.c | 4 +--
fast-import.c| 26
fsck.c | 12
fsck.h | 2 +-
fuzz-pack-headers.c | 4 +--
grep.h | 2 +-
http-push.c | 2 +-
list-objects-filter.c| 2 +-
mailmap.c| 2 +-
match-trees.c| 4 +--
merge-blobs.c| 6 ++--
merge-blobs.h| 2 +-
merge-recursive.c| 4 +--
notes-cache.c| 2 +-
notes-merge.c| 4 +--
notes.c | 6 ++--
object-store.h | 20 ++--
object.c | 4 +--
object.h | 2 +-
pack-check.c | 2 +-
pack-objects.h | 14 -
pack.h | 2 +-
packfile.c | 40
packfile.h | 8 ++---
patch-delta.c| 8 ++---
range-diff.c | 2 +-
read-cache.c | 48 ++---
ref-filter.c | 30 +-
remote-testsvn.c | 4 +--
rerere.c | 2 +-
sha1-file.c | 66
sha1dc_git.c | 2 +-
sha1dc_git.h | 2 +-
streaming.c | 12
streaming.h | 2 +-
submodule-config.c | 2 +-
t/helper/test-delta.c| 2 +-
tag.c| 6 ++--
tag.h| 2 +-
tree-walk.c | 14 -
tree.c | 2 +-
xdiff-interface.c| 4 +--
xdiff-interface.h| 4 +--
85 files changed, 391 insertions(+), 384 deletions(-)
diff --git a/apply.c b/apply.c
index 3703bfc8d0..5e11b85d17 100644
--- a/apply.c
+++ b/apply.c
@@ -3096,7 +3096,7 @@ static int apply_binary_fragment(struct apply_state
*state,
struct patch *patch)
{
struct fragment *fragment = patch->fragments;
- unsigned long len;
+ size_t len;
void *dst;
Re: [PATCH v1 8/9] diff --color-moved-ws: modify allow-indentation-change
Hi Stefan
On 16/11/2018 21:47, Stefan Beller wrote:
On Fri, Nov 16, 2018 at 3:04 AM Phillip Wood wrote:
From: Phillip Wood
Currently diff --color-moved-ws=allow-indentation-change does not
support indentation that contains a mix of tabs and spaces. For
example in commit 546f70f377 ("convert.h: drop 'extern' from function
declaration", 2018-06-30) the function parameters in the following
lines are not colored as moved [1].
-extern int stream_filter(struct stream_filter *,
-const char *input, size_t *isize_p,
-char *output, size_t *osize_p);
+int stream_filter(struct stream_filter *,
+ const char *input, size_t *isize_p,
+ char *output, size_t *osize_p);
This commit changes the way the indentation is handled to track the
visual size of the indentation rather than the characters in the
indentation. This has they benefit that any whitespace errors do not
s/they/the/
Thanks, well spotted
interfer with the move detection (the whitespace errors will still be
highlighted according to --ws-error-highlight). During the discussion
of this feature there were concerns about the correct detection of
indentation for python. However those concerns apply whether or not
we're detecting moved lines so no attempt is made to determine if the
indentation is 'pythonic'.
[1] Note that before the commit to fix the erroneous coloring of moved
lines each line was colored as a different block, since that commit
they are uncolored.
Signed-off-by: Phillip Wood
---
Notes:
Changes since rfc:
- It now replaces the existing implementation rather than adding a new
mode.
- The indentation deltas are now calculated once for each line and
cached.
- Optimized the whitespace delta comparison to compare string lengths
before comparing the actual strings.
- Modified the calculation of tabs as suggested by Stefan.
- Split out the blank line handling into a separate commit as suggest
by Stefan.
- Fixed some comments pointed out by Stefan.
diff.c | 130 +
t/t4015-diff-whitespace.sh | 56
2 files changed, 129 insertions(+), 57 deletions(-)
diff --git a/diff.c b/diff.c
index c378ce3daf..89559293e7 100644
--- a/diff.c
+++ b/diff.c
@@ -750,6 +750,8 @@ struct emitted_diff_symbol {
const char *line;
int len;
int flags;
+ int indent_off;
+ int indent_width;
So this is the trick how we compute the ws related
data only once per line. :-)
On the other hand, we do not save memory by disabling
the ws detection, but I guess that is not a problem for now.
I did wonder about that, but decided the increase was small compared to
all the strings that are copied when creating the emitted_diff_symbols.
If we want to save memory then we should stop struct
emitted_diff_symbol() from carrying a copy of all the strings.
Would it make sense to have the new variables be
unsigned? (Also a comment on what they are, I
needed to read the code to understand off to be
offset into the line, where the content starts, and
width to be the visual width, as I did not recall
the RFC.)
Yes a comment would make sense. I don't think I have a strong preference
for signed/unsigned, I can change it if you want.
Thanks for looking at these so promptly
Best Wishes
Phillip
+static void fill_es_indent_data(struct emitted_diff_symbol *es)
[...]
+ if (o->color_moved_ws_handling &
+ COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)
+ fill_es_indent_data(>emitted_symbols->buf[n]);
Nice.
By reducing the information kept around to ints, we also do not need
to alloc/free
memory for each line.
+++ b/t/t4015-diff-whitespace.sh
@@ -1901,4 +1901,60 @@ test_expect_success 'compare whitespace delta
incompatible with other space opti
test_i18ngrep allow-indentation-change err
'
+test_expect_success 'compare mixed whitespace delta across moved blocks' '
Looks good,
Thanks!
Stefan
Re: [PATCH v1 7/9] diff --color-moved-ws: optimize allow-indentation-change
On 16/11/2018 20:40, Stefan Beller wrote: On Fri, Nov 16, 2018 at 3:04 AM Phillip Wood wrote: From: Phillip Wood When running git diff --color-moved-ws=allow-indentation-change v2.18.0 v2.19.0 cmp_in_block_with_wsd() is called 694908327 times. Of those 42.7% return after comparing a and b. By comparing the lengths first we can return early in all but 0.03% of those cases without dereferencing the string pointers. The comparison between a and c fails in 6.8% of calls, by comparing the lengths first we reject all the failing calls without dereferencing the string pointers. This reduces the time to run the command above by by 42% from 14.6s to 8.5s. This is still much slower than the normal --color-moved which takes ~0.6-0.7s to run but is a significant improvement. The next commits will replace the current implementation with one that works with mixed tabs and spaces in the indentation. I think it is worth optimizing the current implementation first to enable a fair comparison between the two implementations. Up to here the series looks good and I think we could take it as a preparatory self-standing series. Thanks for looking at these, I think it makes sense to split the series here, the commit message for this patch may want tweaking slightly if we do. (I did wonder about splitting it in two when I submitted it but took the easy way out.) Best Wishes Phillip I'll read on. Thanks, Stefan
Re: insteadOf and git-request-pull output
Jeff King writes: > I suspect it would be less confusing if the rewrite were inverted, like: > > [url "gh:"] > rewriteTo = https://github.com > rewritePrivate > > [url "git://github.com"] > rewriteTo = https://github.com > > where the mapping of sections to rewrite rules must be one-to-one, not > one-to-many (and you can see that the flip side is that we have to > repeat ourselves). > > I hate to introduce two ways of doing the same thing, but maybe it is > simple enough to explain that url.X.insteadOf=Y is identical to > url.Y.rewriteTo=X. I do think people have gotten confused by the > ordering of insteadOf over the years, so this would let them specify it > in whichever way makes the most sense to them. I do admit that the current "insteadOf" was too cute a way to configure this feature and I often have to read it aloud twice before convincing myself I got X and Y right. It would have been cleaner if the definition were in the opposite direction, exactly because you can rewrite a single thing into just one way, but we do want to support that many things mapping to the same, and the approach to use "url.Y.rewriteTo=X" does express it better.
Re: [PATCH v2] read-cache: write all indexes with the same permissions
Christian Couder writes:
> "However, as noted in those commits we'd still create the file as
> 0600, and would just re-chmod it only if core.sharedRepository is set
> to "true" or "all". If core.sharedRepository is unset or set to
> "false", then the file mode will not be changed, so without
> core.splitIndex a system with e.g. the umask set to group writeability
> would work for a group member, but not with core.splitIndex set, as
> group members would not be able to access the shared index file.
That is irrelevant. The repository needs to be configured properly
if it wanted to be used by the members of the group, period.
> It is unfortunately not short lived when core.sharedrepository is
> unset for example as adjust_shared_perm() starts with:
>
> int adjust_shared_perm(const char *path)
> {
> int old_mode, new_mode;
>
> if (!get_shared_repository())
> return 0;
>
> but get_shared_repository() will return PERM_UMASK which is 0 when
> git_config_get_value("core.sharedrepository", ...) returns a non zero
> value which happens when "core.sharedrepository" is unset.
Which is to say, you get an unwanted result when your repository is
not configured properly. It is not a news, and I have no sympathy.
Just configure your repository properly and you'll be fine.
>> > Ideally we'd split up the adjust_shared_perm() function to one that
>> > can give us the mode we want so we could just call open() instead of
>> > open() followed by chmod(), but that's an unrelated cleanup.
>>
>> I would drop this paragraph, as I think this is totally incorrect.
>> Imagine your umask is tighter than the target permission. You ask
>> such a helper function and get "you want 0660". Doing open(0660)
>> would not help you an iota---you'd need chmod() or fchmod() to
>> adjust the result anyway, which already is done by
>> adjust-shared-perm.
>
> It seems to me that it is not done when "core.sharedrepository" is unset.
So? You are assuming that the repository is misconfigured and it is
not set to widen the perm bit in the first place, no?
>> > We already have that minor issue with the "index" file
>> > #leftoverbits.
>>
>> The above "Ideally", which I suspect is totally bogus, would show up
>> whey people look for that keyword in the list archive. This is one
>> of the reasons why I try to write it after at least one person
>> sanity checks that an idea floated is worth remembering.
>
> It was in Ævar's commit message and I thought it might be better to
> keep it so that people looking for that keyword could find the above
> as well as the previous RFC patch.
So do you agree that open(0660) does not guarantee the result will
be group writable, the above "Ideally" is misguided nonsense, and
giving the #leftoverbits label to it will clutter the search result
and harm readers? That's good.
Thanks.
Re: [RFC/PATCH] read-cache: write all indexes with the same permissions
On Sat, Nov 17, 2018 at 07:52:30AM +0100, Christian Couder wrote: > On Fri, Nov 16, 2018 at 8:20 PM Duy Nguyen wrote: > > > > On Fri, Nov 16, 2018 at 8:07 PM SZEDER Gábor wrote: > > > > > With the default 20% threshold a new shared index is written rather > > > frequently with our usual small test-repos: > > > > Side note. Split index is definitely not meant for small repos. > > I very much agree with that. It makes sense to use them only for big > repos and big repos usually don't pass a 20% threshold very often. But our test suite does use very small repositories, so perhaps we have been already testing what Ævar wanted to test? (Though I didn't quite understood what that was; and we likely don't do so explicitly, but only by chance with GIT_TEST_SPLIT_INDEX=1.) > > But > > maybe we should have a lower limit (in terms of absolute number of > > entries) that prevent splitting. This splitting seems excessive. > > I would agree if split index was the default mode or if our goal was > to eventually make it the default mode. Same here. If you don't need split index, i.e. don't have huge repos, then don't enable it in the first place. And if it is enabled in a small repo, then the extra effort to write a new shared index is negligible, and the space wasted for those small files doesn't really matter (though arguably the output from a 'ls .git' would be surprising... which, at the same time, would be a good motivating factor to turn the feature off).
Re: insteadOf and git-request-pull output
On Sat, Nov 17, 2018 at 04:46:22PM +0900, Junio C Hamano wrote: > "brian m. carlson" writes: > > >> $ git request-pull HEAD^ git://foo.example.com/example | grep example > >> ssh://bar.example.com/example > >> > >> I think that if we use the "principle of least surprise," insteadOf > >> rules shouldn't be applied for git-request-pull URLs. > > > > I'd like to point out a different use that may change your view. I have > > an insteadOf alias, gh:, that points to GitHub. Performing the rewrite > > is definitely the right thing to do, since other users may not have my > > alias available. > > > > I agree that in your case, a rewrite seems less appropriate, but I think > > we should only skip the rewrite if the value already matches a valid > > URL. > > It would be tricky to define what a valid URL is, though. Do we > need some way to say "this is a private URL that should not be > given preference when composing a request-pull message"? E.g. > > [url "git://git.dev/"] > insteadOf = https://git.dev/ > > [url "https://github.com/;] > insteadOf = gh: > private > > The former does not mark https://git.dev/ a private one, so a > "request-pull https://git.dev/$thing; would show the original > "https://git.dev/$thing; without rewriting. The latter marks gh: a > private one so "request-pull gh:$thing" would be rewritten before > exposed to the public as "https://github.com/$thing; > > Or something like that? One funny thing about this is that the "private" config key is url.https://github.com.private. But that's the public URL! It makes sense if you think of it as "this rewrite is private". And that would probably serve for most people's needs, though it gets funny when you have multiple conversions: [url "https://github.com/;] insteadOf = gh: insteadOf = git://github.com you may want to share that you are rewriting one of those, but not the other. I suspect it would be less confusing if the rewrite were inverted, like: [url "gh:"] rewriteTo = https://github.com rewritePrivate [url "git://github.com"] rewriteTo = https://github.com where the mapping of sections to rewrite rules must be one-to-one, not one-to-many (and you can see that the flip side is that we have to repeat ourselves). I hate to introduce two ways of doing the same thing, but maybe it is simple enough to explain that url.X.insteadOf=Y is identical to url.Y.rewriteTo=X. I do think people have gotten confused by the ordering of insteadOf over the years, so this would let them specify it in whichever way makes the most sense to them. -Peff
Re: [PATCH v2] read-cache: write all indexes with the same permissions
On Fri, Nov 16, 2018 at 08:25:43PM +0100, Christian Couder wrote: > On Fri, Nov 16, 2018 at 7:29 PM SZEDER Gábor wrote: > > > > On Fri, Nov 16, 2018 at 06:31:05PM +0100, Christian Couder wrote: > > > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh > > > index 2ac47aa0e4..fa1d3d468b 100755 > > > --- a/t/t1700-split-index.sh > > > +++ b/t/t1700-split-index.sh > > > @@ -381,6 +381,26 @@ test_expect_success 'check > > > splitIndex.sharedIndexExpire set to "never" and "now" > > > test $(ls .git/sharedindex.* | wc -l) -le 2 > > > ' > > > > > > +test_expect_success POSIXPERM 'same mode for index & split index' ' > > > + git init same-mode && > > > + ( > > > + cd same-mode && > > > + test_commit A && > > > + test_modebits .git/index >index_mode && > > > + test_must_fail git config core.sharedRepository && > > > + git -c core.splitIndex=true status && > > > + shared=$(ls .git/sharedindex.*) && > > > > I think the command substitution and 'ls' are unnecessary, and > > > > shared=.git/sharedindex.* > > > > would work as well. > > If there is no shared index file with the above we would get: > > shared=.git/sharedindex.* > $ echo $shared > .git/sharedindex.* > > which seems bug prone to me. That's just a non-existing file, for which 'test_modebits' will print nothing, which, in turn, will not match the modebits of '.git/index'. And the "there are more than one shared index files" case is handled by the case statement that was snipped from the email context.
Re: [PATCH v2] read-cache: write all indexes with the same permissions
On Sat, Nov 17, 2018 at 10:29 AM Junio C Hamano wrote:
>
> Christian Couder writes:
>
> > However, as noted in those commits we'd still create the file as 0600,
> > and would just re-chmod it depending on the setting of
> > core.sharedRepository. So without core.splitIndex a system with
> > e.g. the umask set to group writeability would work for the members of
> > the group, but not with core.splitIndex set, as members of the group
> > would not be able to access the shared index file.
>
> I am not sure what the above wants to say.
I tried to improve from Ævar's previous commit message but I agree
that the above is not very clear.
> If we are not making
> necessary call to adjust-shared-perm,
The issue is that adjust_shared_perm() returns immediately when
core.sharedRepository is unset (or false). So when it is unset (or
false), and when the umask is 0022 or 0002 for example, then the index
and the shared index will not have the same permissions because one is
created using open() with mode 0666 and the other with mode 0600.
> then it is irrelevant that the
> lack of the call does not immediately cause an apparent problem for
> users who happens to have non-restrictive group perm bit in their
> umask. Another group member whose umask is tighter will eventually
> use the repository and end up creating a file unreadable to group
> members.
The issue is that a group member with non-restrictive group perm bit
in their umask, like 0022 or 0002, will currently create an unreadable
shared index when using the repo.
I agree that it is much safer to just set core.sharedRepository to
"true" or "all", but maybe in some setups/systems it might be ok to
rely on everyone having non-restrictive group perm bit in their umask.
> Are you saying that we _lack_ necessary call when core.sharedRepository
> is set?
No, I am saying that, when it is unset, adjust_shared_perm() does nothing.
> If so, a commit that fixes such a bug would be the best
> place to have a paragraph like the above. If not, the above description
> simply misleads the readers.
I agree that it is a bit misleading. Maybe something like:
"However, as noted in those commits we'd still create the file as
0600, and would just re-chmod it only if core.sharedRepository is set
to "true" or "all". If core.sharedRepository is unset or set to
"false", then the file mode will not be changed, so without
core.splitIndex a system with e.g. the umask set to group writeability
would work for a group member, but not with core.splitIndex set, as
group members would not be able to access the shared index file.
> > Let's instead make the two consistent by using mks_tempfile_sm() and
> > passing 0666 in its `mode` argument.
>
> On the other hand, this is a relevant description; this patch kills
> an inconsistency that is very short lived (I am assuming that there
> is no bug in the current code before this patch and we make
> necessary calls to adjust-shared-perm when core.sharedrepository is
> set).
It is unfortunately not short lived when core.sharedrepository is
unset for example as adjust_shared_perm() starts with:
int adjust_shared_perm(const char *path)
{
int old_mode, new_mode;
if (!get_shared_repository())
return 0;
but get_shared_repository() will return PERM_UMASK which is 0 when
git_config_get_value("core.sharedrepository", ...) returns a non zero
value which happens when "core.sharedrepository" is unset.
Maybe there is a bug somewhere in adjust_shared_perm() or the
functions it calls, but I don't know this part of the code base much.
> > Note that we cannot use the create_tempfile() function itself that is
> > used to write the main ".git/index" file because we want the XX
> > part of the "sharedindex_XX" argument to be replaced by a pseudo
> > random value and create_tempfile() doesn't do that.
>
> Sure. Pseudo-random-ness is less important than the resulting
> filename being unique. "Because we are asking for a unique file to
> be created, we cannot use create_tempfile() interface that is
> designed to be used to create a file with known name."
>
> But is that really worth saying, I wonder.
I am ok with either your version or removing the above from the commit message.
> > Ideally we'd split up the adjust_shared_perm() function to one that
> > can give us the mode we want so we could just call open() instead of
> > open() followed by chmod(), but that's an unrelated cleanup.
>
> I would drop this paragraph, as I think this is totally incorrect.
> Imagine your umask is tighter than the target permission. You ask
> such a helper function and get "you want 0660". Doing open(0660)
> would not help you an iota---you'd need chmod() or fchmod() to
> adjust the result anyway, which already is done by
> adjust-shared-perm.
It seems to me that it is not done when "core.sharedrepository" is unset.
> > We already have that minor issue with the "index" file
> > #leftoverbits.
>
> The above "Ideally", which I
Re: [PATCH v2] read-cache: write all indexes with the same permissions
Christian Couder writes:
> However, as noted in those commits we'd still create the file as 0600,
> and would just re-chmod it depending on the setting of
> core.sharedRepository. So without core.splitIndex a system with
> e.g. the umask set to group writeability would work for the members of
> the group, but not with core.splitIndex set, as members of the group
> would not be able to access the shared index file.
I am not sure what the above wants to say. If we are not making
necessary call to adjust-shared-perm, then it is irrelevant that the
lack of the call does not immediately cause an apparent problem for
users who happens to have non-restrictive group perm bit in their
umask. Another group member whose umask is tighter will eventually
use the repository and end up creating a file unreadable to group
members.
Are you saying that we _lack_ necessary call when core.sharedRepository
is set? If so, a commit that fixes such a bug would be the best
place to have a paragraph like the above. If not, the above description
simply misleads the readers.
> Let's instead make the two consistent by using mks_tempfile_sm() and
> passing 0666 in its `mode` argument.
On the other hand, this is a relevant description; this patch kills
an inconsistency that is very short lived (I am assuming that there
is no bug in the current code before this patch and we make
necessary calls to adjust-shared-perm when core.sharedrepository is
set).
> Note that we cannot use the create_tempfile() function itself that is
> used to write the main ".git/index" file because we want the XX
> part of the "sharedindex_XX" argument to be replaced by a pseudo
> random value and create_tempfile() doesn't do that.
Sure. Pseudo-random-ness is less important than the resulting
filename being unique. "Because we are asking for a unique file to
be created, we cannot use create_tempfile() interface that is
designed to be used to create a file with known name."
But is that really worth saying, I wonder.
> Ideally we'd split up the adjust_shared_perm() function to one that
> can give us the mode we want so we could just call open() instead of
> open() followed by chmod(), but that's an unrelated cleanup.
I would drop this paragraph, as I think this is totally incorrect.
Imagine your umask is tighter than the target permission. You ask
such a helper function and get "you want 0660". Doing open(0660)
would not help you an iota---you'd need chmod() or fchmod() to
adjust the result anyway, which already is done by
adjust-shared-perm.
> We already have that minor issue with the "index" file
> #leftoverbits.
The above "Ideally", which I suspect is totally bogus, would show up
whey people look for that keyword in the list archive. This is one
of the reasons why I try to write it after at least one person
sanity checks that an idea floated is worth remembering.
> Signed-off-by: Ævar Arnfjörð Bjarmason
> Signed-off-by: Christian Couder
> ---
>
> This is a simpler fix iterating from Ævar's RFC patch and the
> following discussions:
>
> https://public-inbox.org/git/20181113153235.25402-1-ava...@gmail.com/
>
> read-cache.c | 3 ++-
> t/t1700-split-index.sh | 20
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 8c924506dd..ea80600bff 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -3165,7 +3165,8 @@ int write_locked_index(struct index_state *istate,
> struct lock_file *lock,
> struct tempfile *temp;
> int saved_errno;
>
> - temp = mks_tempfile(git_path("sharedindex_XX"));
> + /* Same permissions as the main .git/index file */
> + temp = mks_tempfile_sm(git_path("sharedindex_XX"), 0, 0666);
> if (!temp) {
> oidclr(>base_oid);
> ret = do_write_locked_index(istate, lock, flags);
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index 2ac47aa0e4..fa1d3d468b 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire
> set to "never" and "now"
> test $(ls .git/sharedindex.* | wc -l) -le 2
> '
>
> +test_expect_success POSIXPERM 'same mode for index & split index' '
> + git init same-mode &&
> + (
> + cd same-mode &&
> + test_commit A &&
> + test_modebits .git/index >index_mode &&
> + test_must_fail git config core.sharedRepository &&
> + git -c core.splitIndex=true status &&
> + shared=$(ls .git/sharedindex.*) &&
> + case "$shared" in
> + *" "*)
> + # we have more than one???
> + false ;;
> + *)
> + test_modebits "$shared" >split_index_mode &&
> + test_cmp index_mode split_index_mode ;;
> + esac
> +
Re: [PATCH-2] Change writing style
Jacques Bodin-Hullin writes:
> Subject: Re: [PATCH-2] Change writing style
Let's do the usual ": summary" instead. Perhaps
Subject: parse-options: lose an unnecessary space in an error message
It is obvious why it is done, so I do not see a need for any other
description in the body of the message for this change.
We still need the patch signed-off. cf. Documentation/SubmittingPatches
> ---
> parse-options.c | 4 ++--
> t/t0040-parse-options.sh | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
All changes look good to me.
> diff --git a/parse-options.c b/parse-options.c
> index 3b874a83a0c89..f5ef24155950b 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -352,7 +352,7 @@ static void check_typos(const char *arg, const struct
> option *options)
> return;
>
> if (starts_with(arg, "no-")) {
> - error ("did you mean `--%s` (with two dashes ?)", arg);
> + error("did you mean `--%s` (with two dashes)?", arg);
> exit(129);
> }
>
> @@ -360,7 +360,7 @@ static void check_typos(const char *arg, const struct
> option *options)
> if (!options->long_name)
> continue;
> if (starts_with(options->long_name, arg)) {
> - error ("did you mean `--%s` (with two dashes ?)", arg);
> + error("did you mean `--%s` (with two dashes)?", arg);
> exit(129);
> }
> }
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index 5b0560fa20e34..c442f9ae15ff8 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -222,7 +222,7 @@ test_expect_success 'non ambiguous option (after two
> options it abbreviates)' '
> '
>
> cat >typo.err <<\EOF
> -error: did you mean `--boolean` (with two dashes ?)
> +error: did you mean `--boolean` (with two dashes)?
> EOF
>
> test_expect_success 'detect possible typos' '
> @@ -232,7 +232,7 @@ test_expect_success 'detect possible typos' '
> '
>
> cat >typo.err <<\EOF
> -error: did you mean `--ambiguous` (with two dashes ?)
> +error: did you mean `--ambiguous` (with two dashes)?
> EOF
>
> test_expect_success 'detect possible typos' '
>
> --
> https://github.com/git/git/pull/540
Re: [PATCH v2] read-cache: write all indexes with the same permissions
Christian Couder writes: >> > +test_expect_success POSIXPERM 'same mode for index & split index' ' >> > + git init same-mode && >> > + ( >> > + cd same-mode && >> > + test_commit A && >> > + test_modebits .git/index >index_mode && >> > + test_must_fail git config core.sharedRepository && >> > + git -c core.splitIndex=true status && >> > + shared=$(ls .git/sharedindex.*) && >> >> I think the command substitution and 'ls' are unnecessary, and >> >> shared=.git/sharedindex.* >> >> would work as well. > > If there is no shared index file with the above we would get: > > shared=.git/sharedindex.* > $ echo $shared > .git/sharedindex.* > > which seems bug prone to me. It is not bug *PRONE*, but is already wrong, as the way this variable is used is + case "$shared" in + *" "*) + # we have more than one??? + false ;; + *) + test_modebits "$shared" >split_index_mode && + test_cmp index_mode split_index_mode ;; + esac i.e. we'd think there is a singleton ".git/shared/index.*" and we can feed it to test_modebits. So what you wrote originally is corrrect.
Re: [PATCH v2 1/2] commit: don't add scissors line if one exists in MERGE_MSG
Denton Liu writes:
> If commit.cleanup = scissors is specified, don't produce a scissors line
> if one already exists in the MERGE_MSG file.
Are we already sometimes adding a scissors line in that file? The
impression I was getting was that the change in the step 2/2 is the
only reason why this becomes necessary. In which case this change
makes no sense as a standalone patch and requires 2/2 to be a
sensible change, no?
> @@ -798,7 +807,8 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> struct ident_split ci, ai;
>
> if (whence != FROM_COMMIT) {
> - if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
> + if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
> + !merge_contains_scissors)
> wt_status_add_cut_line(s->fp);
> status_printf_ln(s, GIT_COLOR_NORMAL,
> whence == FROM_MERGE
This one is done before we show a block of text, which looks correct.
> @@ -824,7 +834,8 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> " Lines starting\nwith '%c' will be ignored,
> and an empty"
> " message aborts the commit.\n"),
> comment_line_char);
> else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
> - whence == FROM_COMMIT)
> + whence == FROM_COMMIT &&
> + !merge_contains_scissors)
> wt_status_add_cut_line(s->fp);
> else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> status_printf(s, GIT_COLOR_NORMAL,
The correctness of this one in an if/elseif/else cascade is less
clear. When "contains scissors" logic does not kick in, we just
have a scissors line here (i.e. the original behaviour). Now when
the new logic kicks in, we not just omit the (extra) scissors line,
but also say "Please enter the commit message..." which is the
message used with the "comment line char" mode of the clean-up.
I wonder if this is what you really meant to have instead:
> else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
> - whence == FROM_COMMIT)
> - wt_status_add_cut_line(s->fp);
> + whence == FROM_COMMIT) {
> + if (!merge_contains_scissors)
> + wt_status_add_cut_line(s->fp);
> + }
> else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> status_printf(s, GIT_COLOR_NORMAL,
That is, the only behaviour change in "merge contains scissors"
mode is to omit the cut line and nothing else.
