Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GTIndex improvements and GTRepository+Status tweaks #622

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

GTIndex improvements and GTRepository+Status tweaks #622

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a68584
Numerous GTIndex improvements
slavikus Mar 16, 2017
c7fcb63
GTRepository+Status improvements
slavikus Mar 16, 2017
6088ef6
Merge remote-tracking branch 'upstream/master' into gtindex-add-paths…
slavikus Mar 29, 2017
d497586
Code review feedback fixes for PR #622
slavikus Mar 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 52 additions & 1 deletion ObjectiveGit/GTIndex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@

#import <Foundation/Foundation.h>
#include "git2/types.h"
#include "git2/index.h"

@class GTOID;
@class GTIndexEntry;
@class GTRepository;
@class GTTree;
Expand All @@ -45,6 +47,9 @@ NS_ASSUME_NONNULL_BEGIN
/// The file URL for the index if it exists on disk; nil otherwise.
@property (nonatomic, readonly, copy) NSURL * _Nullable fileURL;

/// The index checksum
@property (nonatomic, readonly, strong) GTOID * _Nullable checksum;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_Nullable is not necessary here : the underlying git_oid is part of the git_index struct.


/// The number of entries in the index.
@property (nonatomic, readonly) NSUInteger entryCount;

Expand Down Expand Up @@ -202,7 +207,53 @@ NS_ASSUME_NONNULL_BEGIN
///
/// Returns `YES` in the event of successful enumeration or no conflicts in the
/// index, `NO` in case of error.
- (BOOL)enumerateConflictedFilesWithError:(NSError **)error usingBlock:(void (^)(GTIndexEntry *ancestor, GTIndexEntry *ours, GTIndexEntry *theirs, BOOL *stop))block;
- (BOOL)enumerateConflictedFilesWithError:(NSError **)error usingBlock:(void (^)(GTIndexEntry * _Nullable ancestor, GTIndexEntry * _Nullable ours, GTIndexEntry * _Nullable theirs, BOOL *stop))block;

/// An enum for use with addPathspecs:flags:error:passingTest: below
/// See index.h for documentation of each individual git_index_add_option_t flag.
typedef NS_OPTIONS(NSInteger, GTIndexAddOptionFlags) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Maybe just GTIndexAddOptions ? That would make for more consistent wordings (e.g. "A combination of GTIndexAddOptionFlags flags" below).

GTIndexAddOptionDefault = GIT_INDEX_ADD_DEFAULT,
GTIndexAddOptionForce = GIT_INDEX_ADD_FORCE,
GTIndexAddOptionDisablePathspecMatch = GIT_INDEX_ADD_DISABLE_PATHSPEC_MATCH,
GTIndexAddOptionCheckPathspec = GIT_INDEX_ADD_CHECK_PATHSPEC
};

/// Add or update index entries matching files in the working directory.
/// This method will immediately fail if the index's repo is bare.
///
/// pathspecs - An `NSString` array of path patterns. (E.g: *.c)
/// If nil is passed in, all index entries will be updated.
/// flags - A combination of GTIndexAddOptionFlags flags
/// block - A block run each time a pathspec is matched; before the index is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"A block that will be called on each match, before the index is changed."
"what the pathspec" => "which pathspec".
"If you pass in NULL nil"

/// added/updated. The `matchedPathspec` parameter is a string indicating
/// what the pathspec (from `pathspecs`) matched. If you pass in NULL
/// in to the `pathspecs` parameter this parameter will be nil.
/// The `path` parameter is a repository relative path to the file
/// about to be added/updated.
/// The `stop` parameter can be set to `YES` to abort the operation.
/// Return `YES` to update the given path, or `NO` to skip it. May be nil.
/// error - When something goes wrong, this parameter is set. Optional.
///
/// Returns `YES` in the event that everything has gone smoothly. Otherwise, `NO`.
- (BOOL)addPathspecs:(NSArray<NSString*> * _Nullable)pathspecs flags:(GTIndexAddOptionFlags)flags error:(NSError **)error passingTest:(BOOL (^ _Nullable )(NSString * _Nullable matchedPathspec, NSString *path, BOOL *stop))block;

/// Remove all matching index entries.
/// This method will immediately fail if the index's repo is bare.
///
/// pathspecs - An `NSString` array of path patterns. (E.g: *.c)
/// If nil is passed in, all index entries will be updated.
/// block - A block run each time a pathspec is matched; before the index is
/// removed. The `matchedPathspec` parameter is a string indicating
/// what the pathspec (from `pathspecs`) matched. If you pass in NULL
/// in to the `pathspecs` parameter this parameter will be nil.
/// The `path` parameter is a repository relative path to the file
/// about to be updated.
/// The `stop` parameter can be set to `YES` to abort the operation.
/// Return `YES` to update the given path, or `NO` to skip it. May be nil.
/// error - When something goes wrong, this parameter is set. Optional.
///
/// Returns `YES` in the event that everything has gone smoothly. Otherwise, `NO`.
- (BOOL)removePathspecs:(NSArray<NSString*> * _Nullable)pathspecs error:(NSError **)error passingTest:(BOOL (^ _Nullable)(NSString * _Nullable matchedPathspec, NSString *path, BOOL *stop))block;

/// Update all index entries to match the working directory.
/// This method will immediately fail if the index's repo is bare.
Expand Down
64 changes: 60 additions & 4 deletions ObjectiveGit/GTIndex.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,14 @@ - (id)initWithGitIndex:(git_index *)index repository:(GTRepository *)repository

#pragma mark Entries

- (GTOID *)checksum {
const git_oid *oid = git_index_checksum(self.git_index);
if (oid != NULL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add braces here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

return [GTOID oidWithGitOid:oid];
else
return nil;
}

- (NSUInteger)entryCount {
return git_index_entrycount(self.git_index);
}
Expand Down Expand Up @@ -308,17 +316,17 @@ - (BOOL)enumerateConflictedFilesWithError:(NSError **)error usingBlock:(void (^)
return NO;
}

GTIndexEntry *blockAncestor;
GTIndexEntry *blockAncestor = nil;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is default behavior and could be omitted.

if (ancestor != NULL) {
blockAncestor = [[GTIndexEntry alloc] initWithGitIndexEntry:ancestor];
}

GTIndexEntry *blockOurs;
GTIndexEntry *blockOurs = nil;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is default behavior and could be omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand the compiler makes it easier by auto-nulling new declarations, I don't think an implicit = nil makes any harm. :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slava, that's unrelated. the rationale is pretty simple (kindergarten rule) — compiler doesn't nullify local (stack allocated) scalars in C-derived languages, there is no such a thing in C as "default behaviour" which zeroes locals. explicit zeroing will prevent propagating of rubbish in case when the next condition is false. that's must have.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually don't care too much about this, but in the ARC transitioning release notes it reads "Using ARC, strong, weak, and autoreleasing stack variables are now implicitly initialized with nil.".

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, nice, I always forget about ARC. then it could be omitted, but explicit nullify looks more semantically clean

if (ours != NULL) {
blockOurs = [[GTIndexEntry alloc] initWithGitIndexEntry:ours];
}

GTIndexEntry *blockTheirs;
GTIndexEntry *blockTheirs = nil;
if (theirs != NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is default behavior and could be omitted.

blockTheirs = [[GTIndexEntry alloc] initWithGitIndexEntry:theirs];
}
Expand All @@ -336,10 +344,58 @@ - (BOOL)enumerateConflictedFilesWithError:(NSError **)error usingBlock:(void (^)
BOOL shouldAbortImmediately;
};

- (BOOL)addPathspecs:(NSArray *)pathspecs flags:(GTIndexAddOptionFlags)flags error:(NSError **)error passingTest:(GTIndexPathspecMatchedBlock)block {
NSAssert(self.repository.isBare == NO, @"This method only works with non-bare repositories.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is up for debate, but I feel like an assert is too much ? As someone who writes a GUI (where people can open mostly anything), I'd prefer to get an error back instead of exit.

What does libgit2 do in that case, return GIT_EBARE ?


__block git_strarray strarray = pathspecs.git_strarray;
@onExit {
if (strarray.count > 0) git_strarray_free(&strarray);
};

struct GTIndexPathspecMatchedInfo payload = {
.block = block,
.shouldAbortImmediately = NO,
};

int returnCode = git_index_add_all(self.git_index, &strarray, (unsigned int)flags, (block != nil ? GTIndexPathspecMatchFound : NULL), &payload);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize it's not from here, but GTIndexPathspecMatchFound sounds strange (I was wondering if it was a snuck-in flag or something).

Can you add Callback to the function name ?

if (returnCode != GIT_OK && returnCode != GIT_EUSER) {
if (error != nil) *error = [NSError git_errorFor:returnCode description:NSLocalizedString(@"Could not add to index.", nil)];
return NO;
}

return YES;
}

- (BOOL)removePathspecs:(NSArray *)pathspecs error:(NSError **)error passingTest:(GTIndexPathspecMatchedBlock)block {
NSAssert(self.repository.isBare == NO, @"This method only works with non-bare repositories.");

__block git_strarray strarray = pathspecs.git_strarray;
@onExit {
if (strarray.count > 0) git_strarray_free(&strarray);
};

struct GTIndexPathspecMatchedInfo payload = {
.block = block,
.shouldAbortImmediately = NO,
};

int returnCode = git_index_remove_all(self.git_index, &strarray, (block != nil ? GTIndexPathspecMatchFound : NULL), &payload);
if (returnCode != GIT_OK && returnCode != GIT_EUSER) {
if (error != nil) *error = [NSError git_errorFor:returnCode description:NSLocalizedString(@"Could not remove from index.", nil)];
return NO;
}

return YES;
}

- (BOOL)updatePathspecs:(NSArray *)pathspecs error:(NSError **)error passingTest:(GTIndexPathspecMatchedBlock)block {
NSAssert(self.repository.isBare == NO, @"This method only works with non-bare repositories.");

const git_strarray strarray = pathspecs.git_strarray;
__block git_strarray strarray = pathspecs.git_strarray;
@onExit {
if (strarray.count > 0) git_strarray_free(&strarray);
};

struct GTIndexPathspecMatchedInfo payload = {
.block = block,
.shouldAbortImmediately = NO,
Expand Down
12 changes: 8 additions & 4 deletions ObjectiveGit/GTRepository+Status.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ typedef NS_OPTIONS(NSInteger, GTFileStatusFlags) {
GTFileStatusDeletedInWorktree = GIT_STATUS_WT_DELETED,
GTFileStatusTypeChangedInWorktree = GIT_STATUS_WT_TYPECHANGE,
GTFileStatusRenamedInWorktree = GIT_STATUS_WT_RENAMED,
GTFileStatusUnreadableInWorktree = GIT_STATUS_WT_UNREADABLE,

GTFileStatusIgnored = GIT_STATUS_IGNORED,
GTFileStatusConflicted = GIT_STATUS_CONFLICTED
};

/// An `NSNumber` wrapped `GTRepositoryStatusOptionsShow` bitmask.
Expand All @@ -57,8 +59,8 @@ extern NSString *const GTRepositoryStatusOptionsFlagsKey;
/// An enum, for use as documented, with the `GTRepositoryStatusOptionsFlagsKey`
/// key.
///
/// See status.h for documentation of each individual flag.
typedef enum {
/// See status.h for documentation of each individual git_status_opt_t flag.
typedef NS_OPTIONS(NSInteger, GTRepositoryStatusFlags) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be NS_ENUM, if it was a enum before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, NS_ENUM implies a single enumeration whereas NS_OPTIONS allow multiple values, especially when it bridges stuff to Swift, so you can do stuff like [ .includeUntracked, .includeIgnored ] natively. By the meaning of the enumeration, it's really an options bitfield, hence the choice of NS_OPTIONS over NS_ENUM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, cool.

GTRepositoryStatusFlagsIncludeUntracked = GIT_STATUS_OPT_INCLUDE_UNTRACKED,
GTRepositoryStatusFlagsIncludeIgnored = GIT_STATUS_OPT_INCLUDE_IGNORED,
GTRepositoryStatusFlagsIncludeUnmodified = GIT_STATUS_OPT_INCLUDE_UNMODIFIED,
Expand All @@ -71,9 +73,11 @@ typedef enum {
GTRepositoryStatusFlagsRenamesFromRewrites = GIT_STATUS_OPT_RENAMES_FROM_REWRITES,
GTRepositoryStatusFlagsSortCaseSensitively = GIT_STATUS_OPT_SORT_CASE_SENSITIVELY,
GTRepositoryStatusFlagsSortCaseInsensitively = GIT_STATUS_OPT_SORT_CASE_INSENSITIVELY,
GTRepositoryStatusFlagsNoRefresh = GIT_STATUS_OPT_NO_REFRESH,
GTRepositoryStatusFlagsUpdateIndex = GIT_STATUS_OPT_UPDATE_INDEX,
GTRepositoryStatusFlagsIncludeUnreadable = GIT_STATUS_OPT_INCLUDE_UNREADABLE,
GTRepositoryStatusFlagsIncludeUnreadableAsUntracked = GIT_STATUS_OPT_INCLUDE_UNREADABLE_AS_UNTRACKED,
} GTRepositoryStatusFlags;
GTRepositoryStatusFlagsIncludeUnreadableAsUntracked = GIT_STATUS_OPT_INCLUDE_UNREADABLE_AS_UNTRACKED
};

/// An `NSArray` of `NSStrings`s to limit the status to specific paths inside
/// the repository. The entries in the array represent either single paths or
Expand Down