Improvement: push failures

[r0mainK]

CONTEXT

Hey guys, I'm using the porcelain module, and I would like to improve the push function. Currently, errors are only raised when the client is created with get_transport_and_path, which is fine, but not for the actual push, where they are caught and an errorlog to stderr:

try:
    client.send_pack(path, update_refs,
                     generate_pack_data=r.object_store.generate_pack_data,
                     progress=errstream.write)
    errstream.write(b"Push to " + remote_location_bytes + b" successful.\n")
except (UpdateRefsError, SendPackError) as e:
    errstream.write(b"Push to " + remote_location_bytes +
                    b" failed -> " + e.message.encode(err_encoding) + b"\n")

The thing is that in my case, and I think in most, the uncaught exceptions are not a real problem,, because usually other operations, such as a clone, or a ls_remote already would have caused them to be raised before any push. However, if the error happens during the push, there is no internal way to know and stop an application at that point, at least not directly, since the exception is caught. I implemented a check that compares local/remotes head, but its suboptimal.

IMPROVEMENTS

So here is how we could easily change this:

1. raise an exception if there is a failure during the push, either once the log has been outputted or instead of it.
2. return a boolean at the end of the operation

[jelmer]

See #613 for an earlier discussion about this. I would be happy for a PorcelainError of some sort to be raised in this situation. Patches welcome :)

[r0mainK]

Okay, will take care of it and link a PR here so this can be closed later. I wanna first check out the other exceptions in the porcelain folder to see if this should be ported to other functions, e.g. clone

[r0mainK]

@jelmer so I've encountered what I think is two bugs/typos (thought I might as well pepify all of porcelain to get to know the code, and spotted them at that point):

in path_to_tree_path

def path_to_tree_path(repo_path: str, path: str) -> bytes:
    os.path.relpath(path, repo_path)
    if os.path.sep != '/':
        path = path.replace(os.path.sep, '/')
    return path.encode(sys.getfilesystemencoding())

should be

def path_to_tree_path(repo_path: str, path: str) -> bytes:
    tree_path = os.path.relpath(path, repo_path)
    if os.path.sep != '/':
        tree_path = tree_path.replace(os.path.sep, '/')
    return tree_path.encode(sys.getfilesystemencoding())

in list_tree/ls_tree

            if stat.S_ISDIR(mode):
                list_tree(store, sha, name)

should be

            if stat.S_ISDIR(mode) and recursive:
                list_tree(store, sha, name)

[jelmer]

The second issue (in ls_tree) is definitely a bug - patches welcome.

I'm not too sure about the first, I need to take a closer look - it may just be that one of the callers is not using path_to_tree_path correctly.

[r0mainK]

Okay thought so, will put in a separate commit asap. Yes, seeing as tests fail I think that is probably the case, I will try looking into it. I guess ETA before PR is a couple days, wanted to do it quicker but haven't found the time yet

[r0mainK]

OKay so for sure there is a problem with how path_to_tree_path was used in check_ignore:
Before:

            if os.path.isabs(path):
                path = os.path.relpath(path, r.path)
            if not no_index and path_to_tree_path(r.path, path) in index:  # relpath is used twice !

After:

            if os.path.isabs(path):
                path = path_to_tree_path(r.path, path)
            if not no_index and path in index:
                continue
            if ignore_manager.is_ignored(path):
                yield path

And this raised a new problem: path need to be a string if passed to ignore_manager.is_ignored, but needs to be a byte to check path in index. So I change to this:

             if not no_index and path.encode(DEFAULT_ENCODING) in index:
                continue`

[r0mainK]

Opened a PR : #633 if you wanna see changes, I haven't added porcelainExceptions yet, probly tonight

[jelmer]

PorcelainError and better error reporting in push have landed.